SOLR-17541: LBSolrClient implementations should agree on 'getClient()' semantics #2899

jdyer1 · 2024-12-09T16:25:28Z

With this PR makes LBHttp2SolrClient maintains an instance of HttpSolrClientBuilderBase per "Base Url". This makes the semantics of LBHttp2SolrClient#getClient consistent with that of the older LBHttpSolrClient.

Behavior changes:

LBHttp2SolrClient generates a Http Solr Client per base url
- (1) at constructon
- (2) whenever a previously-unseen base url is encountered
- NOTE: The Map holding the clients may grow unbounded; there is no cleanup short of callng close().
LBHttp2SolrClient mutates the baseSolrUrl variable of the caller-supplied instance of HttpSolrClientBuilderBase whenever it creates a new Http Solr Client.
Both LBHttp2SolrClient and CloudHttp2SolrClient always own the internal/delegate clients. They are now always closed by us on close().

The following class definitions are changed in the SolrJ public API:

LBHttp2SolrClient
- from: LBHttp2SolrClient<C extends HttpSolrClientBase> extends LBSolrClient
- to: LBHttp2SolrClient<B extends HttpSolrClientBuilderBase<?, ?>> extends LBSolrClient
LBHttp2SolrClient.Builder
- from: Builder<C extends HttpSolrClientBase>
- to: Builder<B extends HttpSolrClientBuilderBase<?, ?>>
All methods on LBHttp2SolrClient.Builder return a Builder<B> instead of a Builder<C>

The following are removed from the SolrJ public API:

method CloudHttp2SolrClient.Builder#withHttpClient
constructor LBHttp2SolrClient.Builder(C solrClient, Endpoint... endpoints)
- replaced with: public Builder(B solrClientBuilder, Endpoint... endpoints)
method HttpJdkSolrClient#requestWithBaseUrl (main only, never released)

The following were not removed, but perhaps should be considered to be marked "internal" or "Experimental"

method Http2SolrClient#requestWithBaseUrl (main only, never released)
interface SolrClientFunction (main only, never released)

…(2) Generate new clients whenever a previously-unseen base url is encountered.

dsmiley · 2024-12-09T19:08:20Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java

+    var client = urlToClient.get(endpoint.toString());
+    if (client == null) {
+      String tmpBaseSolrUrl = solrClientBuilder.baseSolrUrl;
+      solrClientBuilder.baseSolrUrl = endpoint.getBaseUrl();
+      client = solrClientBuilder.build();
+      urlToClient.put(endpoint.getBaseUrl(), client);
+      solrClientBuilder.baseSolrUrl = tmpBaseSolrUrl;
+    }
+    return client;


Prefer calling ConcurrentHashMap.computeIfAbsent or similar to get-then-put because of it's nice atomicity properties, and avoids synchronization needs

dsmiley · 2024-12-09T19:08:33Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java

+    }
+  }
+
+  private synchronized HttpSolrClientBase buildClient(Endpoint endpoint) {


synchronized is guarding what here?

The synchronized keyword was to prevent multiple threads from adding clients for the same Base Url. However, in taking your advice to use computeIfAbsent this removes the need.

dsmiley · 2024-12-09T19:10:08Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java

+    if (builder.solrClientBuilder.urlParamNames == null) {
+      this.urlParamNames = Collections.emptySet();
+    } else {
+      this.urlParamNames = Collections.unmodifiableSet(builder.solrClientBuilder.urlParamNames);


probably fine but I'd prefer Set.copyOf here

dsmiley · 2024-12-09T19:19:49Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java

+    for (HttpSolrClientBase client : urlToClient.values()) {
+      IOUtils.closeQuietly(client);
+    }


could be a one-liner with urlToClient.values().forEach(IOUtils::closeQuiety)

jdyer1 · 2024-12-10T21:30:04Z

As a blocker for this PR, I am getting many seemingly-unrelated solr-core test failures. For instance, PostToolTest#testBasicRun.

Maybe this is similar to the problem I describe in #2242, in that by sharing resources between client and server in a unit test, we get automatic PKI authentication, on which the tests depend. If this speculation is true, this would be merely a test bug.

Otherwise, perhaps solr-core needs to be able to share instances of Http2SolrClient across its internals? The major change here being callers can only pass a Builder to LbHttp2SolrClient and CloudHttp2Solrclient and not a pre-built delegate client.

In either case I do not know how to continue to make progress so any advice is appreciated.

dsmiley · 2024-12-12T07:08:01Z

The problem is that org.apache.solr.handler.component.HttpShardHandlerFactory#setSecurityBuilder is being called but it only affects the built client, not clients built separately (in LB). My suspicion is that your conversion of passing the Builder as a template instead of an existing client should switch back to the client as a template and then remember to call org.apache.solr.client.solrj.impl.Http2SolrClient.Builder#withHttpClient from the template.

jdyer1 · 2024-12-12T16:34:48Z

your conversion of passing the Builder as a template instead of an existing client should switch back to the client as a template

I am not sure this ticket/PR has much value if we continue having the caller pass in a pre-built delegate client instead of a Builder. LBHttp2solrClient needs to create a client per base url, but the caller only supplies one. Yet LBHttp2SolrClientmight not know how to correctly clone that client, keeping in mind LBHttp2solrClient is now generic. It seems to me that passing in the pre-built client is more of a relic of the days before we used Bulders in solrJ, and not a really great API decision. I was hoping to improve that here.

dsmiley · 2024-12-12T19:28:28Z

Okay with me to stay with the builder. Probably add a new method to org.apache.solr.security.HttpClientBuilderPlugin for the builder, overloading setup.

As an aside, I'm totally confused about our authentication plugins, especially PKIAuthenticationPlugin as it apparently has a pivotal role, even when there's know "PKI" going on! CC @janhoy I suspect you might know

gerlowskija · 2024-12-13T15:26:02Z

This LGTM, pending figuring out the test-failure issue and adding a CHANGES.txt entry that highlights the deprecations/removals.

The following were not removed, but perhaps should be considered to be marked "internal" or "Experimental"

+1 - Both seems like good candidates for a "lucene.experimental" annotation. (Presumably there should be a "solr.experimental", but I don't think that exists...)

…-built clients

jdyer1 · 2024-12-13T23:57:46Z

Probably add a new method to org.apache.solr.security.HttpClientBuilderPlugin for the builder,

Thank you for the insight on this. I went ahead and did this, but its a bit ugly. I'd rather not have both, but then again, I want to minimize changes to solr-core with this PR.

Besides, @dsmiley, I appreciate the change to synchronize on the Builder. That was a pretty bad miss on my part!

I also added @lucene.experimental to the two places discussed.

dsmiley · 2024-12-15T16:27:03Z

solr/core/src/java/org/apache/solr/core/HttpSolrClientProvider.java

WDYT @iamsanjay ?

dsmiley · 2024-12-15T16:30:42Z

solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java

@@ -305,16 +306,16 @@ public void init(PluginInfo info) {
            sb);
    int soTimeout =
        getParameter(args, HttpClientUtil.PROP_SO_TIMEOUT, HttpClientUtil.DEFAULT_SO_TIMEOUT, sb);
-
-    this.defaultClient =
+    this.httpClientBuilder =


please name httpSolrClientBuilder -- so as to clarify this isn't an Apache/Jetty HttpClient; it's a SolrClient.

dsmiley · 2024-12-15T16:33:22Z

solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java

-            .build();
-    this.defaultClient.addListenerFactory(this.httpListenerFactory);
-    this.loadbalancer = new LBHttp2SolrClient.Builder<Http2SolrClient>(defaultClient).build();
+            .withListenerFactory(List.of(this.httpListenerFactory));


I noticed an issue. This overwrites the list of listeners that may already be present. Shouldn't we be augmenting, not replacing?

janhoy · 2024-12-15T20:59:49Z

PKIAuthenticationPlugin as it apparently has a pivotal role, even when there's know "PKI" going on!

PKI plugin handles incoming internal requests from other nodes in the cluster. Then there is indeed PKI going on 😉 It is registered whenever any auth plugin is active.

jdyer1 added 11 commits December 4, 2024 14:10

Initial development

5261cd3

fix bugs

bc8c888

javadoc modifications

a249169

tidy / remove nocommit

1f8a307

remove obsolete test

90bf9e2

Fix bug: (1) Only generate a client per base url, not base url+core. …

51676f5

…(2) Generate new clients whenever a previously-unseen base url is encountered.

tidy

db76064

Only hold the internal builder

605bdc7

remove override base url from jdk client

508af0d

fix test

39177b4

Merge branch 'main' into feature/SOLR-17541

35651b8

jdyer1 requested a review from gerlowskija December 9, 2024 16:25

github-actions bot added client:solrj tests cat:search cat:cloud labels Dec 9, 2024

add missing @OverRide

19577dc

dsmiley reviewed Dec 9, 2024

View reviewed changes

implement code review suggestions

ca59536

dsmiley added 2 commits December 12, 2024 01:20

fix getClient/buildClient

67c8b8f

more

7b90862

github-actions bot added the cat:cli label Dec 12, 2024

jdyer1 added 3 commits December 13, 2024 17:44

PKI Authentication Plugin to add the listener factory to subsequently…

1539d97

…-built clients

@lucene.experimental annotations

8f17279

tidy

effedb9

github-actions bot added the cat:security label Dec 13, 2024

remove system.out.println

29a3445

dsmiley reviewed Dec 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17541: LBSolrClient implementations should agree on 'getClient()' semantics #2899

SOLR-17541: LBSolrClient implementations should agree on 'getClient()' semantics #2899

jdyer1 commented Dec 9, 2024

dsmiley Dec 9, 2024

dsmiley Dec 9, 2024

jdyer1 Dec 10, 2024

dsmiley Dec 9, 2024

dsmiley Dec 9, 2024

jdyer1 commented Dec 10, 2024

dsmiley commented Dec 12, 2024

jdyer1 commented Dec 12, 2024

dsmiley commented Dec 12, 2024

gerlowskija commented Dec 13, 2024

jdyer1 commented Dec 13, 2024

dsmiley Dec 15, 2024

dsmiley Dec 15, 2024

dsmiley Dec 15, 2024

janhoy commented Dec 15, 2024

SOLR-17541: LBSolrClient implementations should agree on 'getClient()' semantics #2899

Are you sure you want to change the base?

SOLR-17541: LBSolrClient implementations should agree on 'getClient()' semantics #2899

Conversation

jdyer1 commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdyer1 commented Dec 10, 2024

dsmiley commented Dec 12, 2024

jdyer1 commented Dec 12, 2024

dsmiley commented Dec 12, 2024

gerlowskija commented Dec 13, 2024

jdyer1 commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janhoy commented Dec 15, 2024