SOLR-17771: Have a CloudSolrClient that works with HttpJdkSolrClient #3774

jdyer1 · 2025-10-15T19:19:48Z

This includes changes to CloudHttp2SolrClient to allow it to delegate to any instance of HttpSolrClientBase or HttpSolrClientBuilderBase.

This removes a redundant Builder inner class of CloudSolrclient and migrates usages to CloudHttp2SolrClient.Builder.

A few classes geared for internal server use will now throw IllegalArgumentException if passed a CSC that does not delegate to the Jetty client.

This PR also fixes a bug in HttpjdkSolrClient in that it could not work in some situations for updates to multiple base urls.

This PR is geared for main/10x only. The fix for HttpjdkSolrClient could be backported to 9.x. Also it may be necessary to deprecate the removed CloudSolrclient.Builder class in 9.x.

…er warnings

… base URI

… if classpath contans Jetty Http

Copilot

Pull Request Overview

Introduces broader HTTP client delegation in CloudHttp2SolrClient (supporting Http2SolrClient or HttpJdkSolrClient) and removes the deprecated CloudSolrClient.Builder, updating tests and internal server utilities accordingly. Key changes include refactoring builder usages to CloudHttp2SolrClient.Builder, revising HEAD request logic in HttpJdkSolrClient, and tightening internal APIs to require Jetty-based clients where needed.

Replaces CloudSolrClient.Builder usage across tests and examples with CloudHttp2SolrClient.Builder
Modifies HttpJdkSolrClient HEAD request caching to be per-base URI (but with an implementation flaw)
Adjusts internal server-only classes to validate delegate HTTP client type (Jetty-based)

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
AbstractFullDistribZkTestBase.java	Javadoc updated to reference new builder class.
HttpJdkSolrClientTest.java	Tests adapted to new HEAD tracking map API.
HttpClusterStateSSLTest.java	Switched to CloudHttp2SolrClient builder.
CloudSolrClientMultiConstructorTest.java	Builder migration to CloudHttp2SolrClient.
CloudSolrClientBuilderTest.java	All builder usages updated.
CloudHttp2SolrClientTest.java	Expanded test coverage for multiple delegate clients.
CloudHttp2SolrClientRetryTest.java	Randomized delegate client selection added.
CloudHttp2SolrClientBuilderTest.java	Added tests for Jetty/JDK builders and external clients.
HttpJdkSolrClient.java	Reworked HEAD request logic to per-URI map; potential logical flaw.
CloudSolrClient.java	Removed inner Builder class (API cleanup).
CloudHttp2SolrClient.java	Generalized to HttpSolrClientBase; dynamic client selection.
SolrClientNodeStateProvider.java	Enforces Jetty-based delegate; adjusted request paths.
NodeValueFetcher.java	Updated to use narrowed client accessor.
UsingSolrJRefGuideExamplesTest.java	Examples updated to new builder class.
KafkaCrossDcConsumer.java	Internal client creation migrated to new builder.
RunExampleTool.java	Cloud client construction updated.
libs.versions.toml	Reordered one dependency entry (no functional change).

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

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

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

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

Copilot

Pull Request Overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

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

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

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

dsmiley · 2025-10-16T04:29:33Z

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

    } else if (builder.internalClientBuilder != null) {
      return builder.internalClientBuilder.build();
    } else {
+      try {


I think this logic should be done in a static initializer initializing a boolean constant.

great idea.

dsmiley · 2025-10-16T04:35:49Z

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

+    return maybeTryHeadRequestSync(uriNoQueryParams);
+  }
+
+  protected final Map<URI, Boolean> headSucceededByBaseUri =


do we really need to differentiate by base URI? Meaning, in practice, is that really a concern?

CloudHttp2SolrClientTest fails without this. It is not that one shard is http/1 and the other is http/2, it is that it seems the JDK code also is tracking which hosts it knows for sure using http/2. So we were sending the HEAD request to the first shard and the JDK knew that shard was http/2, but for the second shard, it would still be sending a jetty-incompatible upgrade request. This change sends the HEAD request once to every shard and then all the components are happy.

dsmiley · 2025-10-16T04:38:28Z

.../solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientNodeStateProvider.java

  private Map<String, Map> nodeVsTags = new HashMap<>();

  public SolrClientNodeStateProvider(CloudHttp2SolrClient solrClient) {
+    if (!(solrClient.getHttpClient() instanceof Http2SolrClient)) {


why bother insisting on this?

these call the very special "requestwithbaseUrl" method on the jetty client. I was unable to quickly create a common api for this with both clients so I decided for now we just need to enforce the jetty client. In practice, I would think these are only used internally so nothing is going to pass the jdk client here.

dsmiley · 2025-10-16T04:45:13Z

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

    } else {
+      try {
+        Class.forName("org.eclipse.jetty.client.HttpClient");
+      } catch (ClassNotFoundException e) {


have you tried if this actually works? You'd need to attempt on some other project. Recently I used the Solr MCP project for a similar test to see if excluding Apache HttpClient works.

I manually tested that it correctly picks the jdk client when jetty is absent. I also added a unit test for the case where jetty is present. I wasn't brave enough to try and write the unit test with jetty absent.

…olrClient.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

- decide default delegate client class in static intalizer (suggested by dsmiley) - add unit test for the cae where jetty client shuld be default delegat client (cannot be tested so easy for djdk client)

dsmiley

Overall looks good to me!
It'd be good to get this into Solr 10. Maybe 9?

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

dsmiley · 2025-10-16T20:39:42Z

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


    /**
-     * Set the internal {@link Http2SolrClient}.
+     * Set the internal client Solr HTTP client.


That sounds clumsy to me. Maybe just refer to the type -- HttpSolrClientBase?

…p2SolrClient.java Co-authored-by: David Smiley <dsmiley@apache.org>

…3774)

jdyer1 added 19 commits October 13, 2025 12:00

Get rid of redundant Builder class on abstract CloudSolrClient

17ba151

Make CloudHttp2SolrClient generic; update references to remove compil…

b5c6c99

…er warnings

remove generics; test with jdk client

f55e6e4

Enhance JDK client to track if it has issued a HEAD request by unique…

4f31577

… base URI

fix test randomization

6923675

make it final

dc9f389

use jdk client somtimes in retry test

d4bea59

tidy

1fe02cb

remove system.out.println

dd98e51

Require that sCNSP users pass-in CSC with Http2SolrCLient

06b72c9

javadoc update

f4e8d04

tidy

35831c5

if not client or builder passed, decide between H2SC or HJSc based on…

0e50350

… if classpath contans Jetty Http

tidy

474fe5e

make precommit happy

9835ec0

Merge branch 'main' into feature/SOLR-17771

3577dfa

fix two more classes that require http2solrclient delegate w/csc

398a9b6

flub the metrics assert a little!

d2fbe96

tidy

1bf8223

jdyer1 requested review from Copilot and dsmiley October 15, 2025 19:19

github-actions bot added documentation Improvements or additions to documentation dependencies Dependency upgrades test-framework client:solrj tool:build tests cat:cloud cat:cli labels Oct 15, 2025

jdyer1 requested a review from gerlowskija October 15, 2025 19:20

jdyer1 requested a review from mlbiscoc October 15, 2025 19:20

Copilot AI reviewed Oct 15, 2025

View reviewed changes

jdyer1 added 4 commits October 15, 2025 14:24

fix randomization bug - suggested by copilot

da66549

really strip of query params - suggested by copilot

26ad6a9

null the variable on teardown - suggested by copilot

10de799

tidy

042eae6

jdyer1 requested a review from Copilot October 16, 2025 00:02

Copilot AI reviewed Oct 16, 2025

View reviewed changes

dsmiley reviewed Oct 16, 2025

View reviewed changes

jdyer1 and others added 3 commits October 16, 2025 14:52

Update solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkS…

6ca39b4

…olrClient.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

- make doc comments genric (suggested by copilot)

fc91fbe

- decide default delegate client class in static intalizer (suggested by dsmiley) - add unit test for the cae where jetty client shuld be default delegat client (cannot be tested so easy for djdk client)

tidy

d47d922

dsmiley reviewed Oct 16, 2025

View reviewed changes

jdyer1 and others added 2 commits October 17, 2025 10:37

Update solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHtt…

2c83145

…p2SolrClient.java Co-authored-by: David Smiley <dsmiley@apache.org>

fix awkward wording

7c5717f

jdyer1 merged commit aad0c4b into apache:main Oct 17, 2025
4 of 5 checks passed

asf-gitbox-commits pushed a commit that referenced this pull request Oct 17, 2025

SOLR-17771: Have a CloudSolrClient that works with HttpJdkSolrClient (#…

554bb7d

…3774)

asf-gitbox-commits pushed a commit that referenced this pull request Oct 17, 2025

SOLR-17771: Have a CloudSolrClient that works with HttpJdkSolrClient (#…

15112d1

…3774)

SOLR-17771: Have a CloudSolrClient that works with HttpJdkSolrClient #3774

SOLR-17771: Have a CloudSolrClient that works with HttpJdkSolrClient #3774

Uh oh!

Conversation

jdyer1 commented Oct 15, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants