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 LBHttp2SolrClient.Builder#withListenerFactory
- renamed to LBHttp2SolrClient.Builder#withListenerFactories (replace any existing)
- add LBHttp2SolrClient.Builder#addListenerFactory (add to existing)
method HttpJdkSolrClient#requestWithBaseUrl (main only, never released)

The following are marked @lucene.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-16T16:24:37Z

I am curious to know opinions as to how to handle deprecations here. This PR introduces several API changes (see description) but most of the replacement APIs will never exist in any 9.x version. We can add a @deprecated annotation to CloudHttp2SolrClient.Builder#withHttpClient, with a suggestion to use #withInternalClientBuilder instead. Beyond that, is there anything else we can/should do in a theoretical Solr 9.9 to warn users of the upcoming changes?

dsmiley

So this would be Solr 10?

dsmiley · 2024-12-17T01:29:43Z

solr/core/src/java/org/apache/solr/cloud/ZkController.java

@@ -872,15 +868,14 @@ public SolrCloudManager getSolrCloudManager() {
      if (cloudManager != null) {
        return cloudManager;
      }
-      http2SolrClient =
+      var httpClientBuilder =


again, should have a name like "solrClientBuilder" (maybe adding "http") to clarify this is building a SolrClient not an HttpClient.

dsmiley · 2024-12-17T01:30:46Z

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

  private final InstrumentedHttpListenerFactory trackHttpSolrMetrics;

  HttpSolrClientProvider(UpdateShardHandlerConfig cfg, SolrMetricsContext parentContext) {
    trackHttpSolrMetrics = new InstrumentedHttpListenerFactory(getNameStrategy(cfg));
    initializeMetrics(parentContext);

-    Http2SolrClient.Builder httpClientBuilder =
-        new Http2SolrClient.Builder().withListenerFactory(List.of(trackHttpSolrMetrics));
+    this.httpClientBuilder = new Http2SolrClient.Builder().addListenerFactory(trackHttpSolrMetrics);


name httpSolrClientBuilder or similar

dsmiley · 2024-12-17T01:40:02Z

solr/core/src/java/org/apache/solr/security/HttpClientBuilderPlugin.java

@@ -34,4 +34,8 @@ public interface HttpClientBuilderPlugin {
  public SolrHttpClientBuilder getHttpClientBuilder(SolrHttpClientBuilder builder);

  public default void setup(Http2SolrClient client) {}
+
+  public default void setup(Http2SolrClient.Builder httpClientBuilder, Http2SolrClient client) {


Maybe these should be split up; setup a client and builder separately (2 methods)? Feels weird to take both in setup().

Yes, I agree separate methods would be nicer. But the implementing classes do things on setup that should only be done once. I settled on this ugly API with hopes this is the least-trappy thing to leave for future developers.

Can you point me at such so I can appreciate the problem better?

dsmiley · 2024-12-17T01:42:30Z

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

@@ -54,8 +53,8 @@ public class CloudHttp2SolrClient extends CloudSolrClient {
   */
  protected CloudHttp2SolrClient(Builder builder) {
    super(builder.shardLeadersOnly, builder.parallelUpdates, builder.directUpdatesToLeadersOnly);
-    this.clientIsInternal = builder.httpClient == null;
-    this.myClient = createOrGetHttpClientFromBuilder(builder);
+    var httpClientBuilder = createOrGetHttpClientBuilder(builder);


httpSolrClientBuilder

dsmiley · 2024-12-17T01:47:03Z

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

-     * @param httpClient http client
-     * @return this
-     */
-    public Builder withHttpClient(Http2SolrClient httpClient) {


not sure we want to take this away from users

This is the most controversial part of this PR, and I do not want to merge a potentially disruptive API change like this without buy-in. I do not think this is much of a burden though, because the Builder has withInternalClientBuilder, which users should be able to easily migrate to use.

The purpose of forcing this is that CloudHttp2SolrClient in turn passes this Client(Builder) to LBHttp2SolrClient. But as noted before, LBHttp2SolrClient doesn't really use this Client either; it clones it using Http2SolrClient.builder#withHttpClient. Yet we've made LBHttp2SolrClient generic so this means that any subclass of HttpSolrClientBase needs to also have a way to be cloned. Personally I think clone methods like this are brittle and a maintenance burden, and I don't want to need to write one for HttpJdkSolrClient. It also seems like a better API: by telling the user we want a Builder, we signal that we will create Clients as needed. Passing in an already-built Client signals that we will use that particular one, which is not always the case here.

it clones it using Http2SolrClient.builder#withHttpClient

which still exists, so if a user somehow has a client but not a builder but needs one due to the API change you introduce, they have this option.

by telling the user we want a Builder, we signal that we will create Clients as needed

Well written James!

+1

dsmiley · 2024-12-17T02:58:37Z

solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java

-    try (MockHttpSolrClient client = new MockHttpSolrClient("http://base.url", b);
-        LBHttp2SolrClient<MockHttpSolrClient> testClient =
-            new LBHttp2SolrClient.Builder<>(client, ep1, ep2).build()) {
+    var httpClientBuilder =


httpSolrClientBuilder

dsmiley · 2024-12-17T02:59:05Z

solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java

-    try (MockHttpSolrClient client = new MockHttpSolrClient("http://base.url", b);
-        LBHttp2SolrClient<MockHttpSolrClient> testClient =
-            new LBHttp2SolrClient.Builder<>(client, ep1, ep2).build()) {
+    var httpClientBuilder =


httpSolrClientBuilder

dsmiley · 2024-12-17T02:59:31Z

solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java

-    try (MockHttpSolrClient client = new MockHttpSolrClient("http://base.url", b);
-        LBHttp2SolrClient<MockHttpSolrClient> testClient =
-            new LBHttp2SolrClient.Builder<>(client, ep1, ep2).build()) {
+    var httpClientBuilder =


httpSolrClientBuilder

dsmiley · 2024-12-17T03:05:13Z

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

-    public Http2SolrClient.Builder withListenerFactory(List<HttpListenerFactory> listenerFactory) {
-      this.listenerFactory = listenerFactory;
+    /**
+     * specify a listener factory, which will be appened to any existing values.


Suggested change

* specify a listener factory, which will be appened to any existing values.

* Specify a listener factory, which will be appended to any existing values.

dsmiley · 2024-12-17T03:11:39Z

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

@@ -94,35 +97,63 @@
 *
 * @since solr 8.0
 */
-public class LBHttp2SolrClient<C extends HttpSolrClientBase> extends LBSolrClient {
+public class LBHttp2SolrClient<B extends HttpSolrClientBuilderBase<?, ?>> extends LBSolrClient {


an aside -- I wonder if the use of generics here is more annoying than helpful. We aren't compelled to use generics.

I worry too I have gone down the wrong road with this. On the other hand, I do not want consider this with this PR.

in another PR then.

dsmiley · 2024-12-17T03:15:37Z

RE deprecations: A good call out but I don't think we should worry much about deprecations if we can't provide the alternative. At least not for LBSolrClient which is rather internal, I think.

jdyer1 · 2024-12-17T23:25:15Z

So this would be Solr 10?

Yes. When I read the description on this ticket, it seemed to me a good opportunity to improve our API with the upcoming major release. It seems wrong we clone the passed-in client. This is especially important if we want to eventually have merely one LB Client, one Cloud Client and one Cluster State Provider that can take any subclass of HttpSolrClientBase.

dsmiley · 2024-12-18T04:52:26Z

solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java

+    setup(client, null);
+  }
+
+  private void setup(Http2SolrClient client, Http2SolrClient.Builder builder) {


why have this separate and with different parameter order? In other words, lets make this one the @Override with builder then client. Remove the 4 lines earlier on 377.

dsmiley · 2024-12-18T04:58:36Z

solr/core/src/java/org/apache/solr/security/HttpClientBuilderPlugin.java

@@ -34,4 +34,8 @@ public interface HttpClientBuilderPlugin {
  public SolrHttpClientBuilder getHttpClientBuilder(SolrHttpClientBuilder builder);

  public default void setup(Http2SolrClient client) {}
+
+  public default void setup(Http2SolrClient.Builder httpClientBuilder, Http2SolrClient client) {


Can you point me at such so I can appreciate the problem better?

jdyer1 · 2024-12-18T20:07:21Z

Can you point me at such so I can appreciate the problem better?

My apologies, I think I misstated what is going on earlier. PKIAuthenticationPlugin on setup injects a RequestResponseListener into the passed-in Client. I wanted to ensure the same Listener gets injected both in the Builder and the Client, in case it matters.

dsmiley · 2024-12-18T20:09:51Z

Since the direction is SolrClient immutability, I think this auth/PKI setup method should only the builder (mutable). Is this realistic?

jdyer1 · 2024-12-18T21:31:18Z

Since the direction is SolrClient immutability, I think this auth/PKI setup method should only the builder (mutable). Is this realistic?

I fully agree this is the right direction to take. Unfortunately this probably requires a minor redesign to both UpdateShardHandler and HttpSolrClientProvider, which is out-of-scope here.

dsmiley

Okay then let's leave a TODO comment to communicate this direction that's out of scope. Maybe even create a JIRA issue and refer to it in the TODO. it'd be good if someone addressed that for Solr 10.

Just some nitpicks on comments / naming here but I'm +1 to merge otherwise.

dsmiley · 2024-12-18T22:19:07Z

solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java

  }

-  private void setup(Http2SolrClient client, Http2SolrClient.Builder builder) {
+  @Override
+  public void setup(Http2SolrClient.Builder httpClientBuilder, Http2SolrClient client) {


Suggest naming this param "builder" as it was, or "httpSolrClientBuilder". Note "client" is simply "client".

dsmiley

CHANGES.txt will be interesting here; a number of things happened.

jdyer1 · 2024-12-19T15:37:10Z

I added a CHANGES.txt entry to this PR and plan to merge this early next week. I also will separately add a deprecation on CloudHttp2SolrClient#withHttpClient in branch_9x, to be released with Solr 9.9.

…tory to LBHttp2SolrClient.Builder#withListenerFactories

…Client()' semantics (#2899)" This reverts commit d3e57aa.

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

jdyer1 added 3 commits December 16, 2024 09:12

Remove unnecessary null check

6176a99

tidy

783f815

small change to doc comment

e8c998d

dsmiley reviewed Dec 17, 2024

View reviewed changes

fix typo

e035a0c

Better variable names

99282aa

dsmiley reviewed Dec 18, 2024

View reviewed changes

remove silly duplicate method

45b9e10

dsmiley reviewed Dec 18, 2024

View reviewed changes

jdyer1 added 5 commits December 19, 2024 08:16

variable rename

7df3c3b

Merge branch 'main' into feature/SOLR-17541

90bd2ec

TODO on HttpClientBuilderPlugin

f984d12

CHANGES.txt

28aec3b

tidy

30dcdb3

jdyer1 mentioned this pull request Dec 19, 2024

SOLR-17541: Deprecate CloudHttp2SolrClient.Builder#withHttpClient (Solr 9.9) #2920

Closed

CHANGES.txt deprecation notice for Solr 9.9

730aad0

dsmiley approved these changes Dec 19, 2024

View reviewed changes

jdyer1 added 2 commits December 19, 2024 12:00

CHANGES.txt: mention rename LBHttp2SolrClient.Builder#withListenerFac…

071fb04

…tory to LBHttp2SolrClient.Builder#withListenerFactories

Merge branch 'main' into feature/SOLR-17541

05eb1ea

jdyer1 merged commit d3e57aa into apache:main Dec 23, 2024
4 checks passed

jdyer1 deleted the feature/SOLR-17541 branch December 23, 2024 15:04

asfgit pushed a commit that referenced this pull request Dec 23, 2024

Revert "SOLR-17541: LBSolrClient implementations should agree on 'get…

704f2fe

…Client()' semantics (#2899)" This reverts commit d3e57aa.

	* specify a listener factory, which will be appened to any existing values.
	* Specify a listener factory, which will be appended to any existing values.

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

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

Conversation

jdyer1 commented Dec 9, 2024 • edited Loading

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 16, 2024

dsmiley left a 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

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 Dec 17, 2024 • edited Loading

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

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

dsmiley commented Dec 17, 2024

jdyer1 commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdyer1 commented Dec 18, 2024

dsmiley commented Dec 18, 2024

jdyer1 commented Dec 18, 2024

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley left a comment

Choose a reason for hiding this comment

jdyer1 commented Dec 19, 2024

jdyer1 commented Dec 9, 2024 •

edited

Loading

jdyer1 Dec 17, 2024 •

edited

Loading