SOLR-17516: LBHttpSolrClient: support HttpJdkSolrClient (multiple class verson) #2805
Conversation
| .withDefaultCollection(solr[0].getDefaultCollection()) | ||
| .setAliveCheckInterval(500, TimeUnit.MILLISECONDS) | ||
| .build()) { | ||
| try (var h = client(baseSolrEndpoints)) { |
There was a problem hiding this comment.
h? the type isn't clear enough to use 'var' if the name is merely 'h'.
| } | ||
| } | ||
|
|
||
| private static class LBClientHolder implements AutoCloseable { |
There was a problem hiding this comment.
Wouldn't closing lbClient close the delegate? If it doesn't, shouldn't it? Then LBClientHolder could go away.
There was a problem hiding this comment.
Well, looking at LBSolrClient#close (from 2018) -- from which all 3 implementations inherit -- it doesn't seem to. And before I modified this test, it had both the inner and outer clients in the autoclosable try/catch. I agree that this seems wrong and perhaps was an oversight long ago. But then again, because we make the user create their own inner client as a separate step, perhaps it was thought it is the opener's responsibility to explicitly close.
There was a problem hiding this comment.
we make the user create their own inner client as a separate step, perhaps it was thought it is the opener's responsibility to explicitly close
Ah; okay that makes sense; I didn't look previously.
There was a problem hiding this comment.
instead of a holder, could client return an anonymous subclass that propagates the close?
There was a problem hiding this comment.
The holder is just to avoid copy/paste in these test methods. I'd rather test the actual class and not a subclass.
| import org.slf4j.MDC; | ||
|
|
||
| /** | ||
| * LBHttp2SolrClient or "LoadBalanced LBHttp2SolrClient" is a load balancing wrapper around {@link |
There was a problem hiding this comment.
references to Http2SolrClient here should be replaced; could just refer to an Http based SolrClient.
| * | ||
| * @since solr 8.0 | ||
| */ | ||
| public abstract class LBHttpSolrClientBase<C extends HttpSolrClientBase> extends LBSolrClient { |
There was a problem hiding this comment.
Do we need subclassing? In other words, can we have a general (not abstract) LB SolrClient that accepts an HttpSolrClientBase ?
There was a problem hiding this comment.
Yes, we do not need to subclass and I agree a generic LBHttpSolrClient would be better than this. Doing this would be an API change, and although we can change APIs with 10.0 coming, I wonder if it is worth annoying our users for what seems like a very slight improvement?
There was a problem hiding this comment.
I think yes; if we can't change major things in a major API then.... ? wow.
Also, again, few users use LbSolrClient; it's more of an internal SolrJ component. I say this from experience only; we don't officially communicate such.
| } | ||
| } | ||
|
|
||
| private static class LBClientHolder implements AutoCloseable { |
There was a problem hiding this comment.
instead of a holder, could client return an anonymous subclass that propagates the close?
|
Instead of subclasses of LB....SolrClient, ...Cloud...SolrClient, ...Http...ClusterStateProvider, we can have concrete impls that delegate to a SolrClient that is provided. It's a vision anyway; hopefully achievable? CC @janhoy @gerlowskija as well |
Isn't this a bit much to aim for in 10.0? I'd rather get a |
|
Users generally don't refer to the LB client nor ClusterStateProvider; we shouldn't constrain ourselves, especially in a major release. I'll propose a dev list discussion on how users choose between JDK vs Jetty for CloudSolrClient (which doesn't exist yet but if it did....) |
|
I am torn about what to do about the lengthy doc-comment which was copy/pasted from |
I'm not sure I agree; it's how the issue is approached. Maybe there's a sunk-cost issue here |
Agree, wouldn't care too much about breaking these two APIs. Also, if there is will and energy to refactor things for 10.0, it is better for users to take the hit in v10 than doing yet another change for v11. |
jdyer1
changed the title
|
I opened a better PR #2828 with a generic client. Please review/compare. |
|
superseded by #2828. |
This is a minimal PR to add Load Balancing functionality for the HttpJdkSolrClient. Included is test refactoring, adding the *JdkClient as a random choice for the integration test.
The result is not ideal: We have two layers of abstract classes and 3 implementations. Of the 3 implementations 1 is deprecated and the other two share all code. If we really are getting rid of the deprecated client for 10.0, we could easily consolidate in the future. A generic LB Solr Client is likely best but users may find it an annoying API change that only yields marginal improvement.