Skip to content

Conversation

epugh
Copy link
Contributor

@epugh epugh commented Oct 7, 2025

https://issues.apache.org/jira/browse/SOLR-17931

Description

Deal with deprecation tag.

Solution

Move SolrClientCache to live in the ObjectCache, which is the path recommended by the deprecation tag.

I left in a lot of the comments from Claude, however would prune it down if this patch looks good to folks.

Tests

No new tests, re-ran existing test.

@epugh epugh requested review from dsmiley and iamsanjay October 7, 2025 11:33
* @deprecated likely to simply be moved to the ObjectCache so as to not be used
*/
@Deprecated
public SolrClientCache getSolrClientCache() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No; this misses the point. I don't like this method's existence; it's too tempting to use it when usually you shouldn't use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate more on what the problem with this method is? Is there a more general approach that we are supposed to take for getting clients? at least to me, it appears super useful... If we we have a client created, great, use it, and if not, it creates it...

Only six classes call it, so if we want to eliminate it, it doesn't seem unsurmountable. Just unclear on what we would replace it with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the javadocs don't already answer your question, I failed to make them clear enough. Let me flip this inquiry around... hey Eric, why do you think we need a cache of clients at all? Why cache them? What's wrong with our existing clients (linked to in javadocs)?

note: distributed file store usage could easily switch to coreContainer.getDefaultHttpSolrClient().requestWithBaseUrl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, after re-reading and more context.. ( I sound like a LLM!) I think what you are saying is that we could replace most calls with one of:

   * @see #getDefaultHttpSolrClient()
   * @see ZkController#getSolrClient()
   * @see Http2SolrClient#requestWithBaseUrl(String, String, SolrRequest)

and that would work? I'll take a looksee at that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also fine with the PR, TBH... enough callers of it want a SolrClientCache that it'd be annoying to outright remove it. And it's not a big deal if a plugin or something uses it when they could have used an alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants