[SOLR-18130][WIP] Unified connection string CloudSolrClient.Builder implementation by vvova15 · Pull Request #4260 · apache/solr

vvova15 · 2026-04-01T18:48:11Z

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

Description

This patch introduces universal connection string support for CloudSolrClient.Builder
and replaces the existing usage in SolrClientCache.

Current state:

getCloudSolrClient now works with both connection types.
Variables and parameters are still called zkHost.

Proposed next steps:

Add an optional second parameter "connectionString" while retaining zkHost
for backward compatibility.
Ensure full support for both ZooKeeper and HTTP connection types in solrj-streaming.

This is a work-in-progress PR for early feedback and discussion.

Solution

Added a CloudSolrClient.Builder constructor that handles a connection string that can be either a comma-separated list of HTTP(s) URLs in Solr or a list of Zookeeper hosts ending with /chroot.
Replaced the use of the CloudSolrClient.Builder constructor that accepts zookeeper parameters in favor of a constructor with a generic connection string.

Tests

Added unit tests in CloudHttp2SolrClientTest, SolrClientCacheTest
Written new unit test CloudClientConnectionStringTest

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide
I have added a changelog entry for my change

dsmiley

Great start! Heck; this is even contributable as-is... say maybe with some additonal usages of this nifty constructor.

@epugh in the CLI (or anywhere you know of) can this be used? From my experience, this is useful for clients creating CloudSolrClient in a generic way with respect to where the state comes from.

dsmiley · 2026-04-02T00:50:00Z

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

+     * <p>The parser automatically detects the mode based on the presence of a scheme:
+     *
+     * <ul>
+     *   <li>If any part starts with {@code http://} or {@code https://}, the entire string is
+     *       treated as a list of HTTP(S) URLs.
+     *   <li>Otherwise, it treated as a ZooKeeper connection string (with optional chroot)
+     * </ul>
+     *
+     * <p><b>Important:</b> Mixed schemes (e.g. zk hosts + HTTP URLs) are not allowed and will
+     * result in an error when building the client.
+     *
+     * <p>Usage examples:
+     *
+     * <pre>{@code
+     * // ZooKeeper with chroot
+     * new CloudSolrClient.Builder("zk1:2181,zk2:2181,zk3:2181/solr");
+     *
+     * // ZooKeeper without chroot
+     * new CloudSolrClient.Builder("zk1:2181,zk2:2181,zk3:2181");
+     *
+     * // Direct HTTPS connections
+     * new CloudSolrClient.Builder("https://solr1:8983/solr,https://solr2:8983/solr");
+     * }</pre>
+     *


I would drop all this honestly

dsmiley · 2026-04-02T00:51:28Z

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

+     * @param connectionString a string specifying either ZooKeeper connection string or HTTP(S)
+     *     Solr URLs
+     * @throws IllegalArgumentException if string is null, empty, or malformed
+     * @see CloudClientConnectionString#parse(String) for parsing logic


no; that class seems should be a private detail, not a public API

dsmiley · 2026-04-02T00:52:11Z

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

+    public Builder(String connectionString) {
+      CloudClientConnectionString connStr = CloudClientConnectionString.parse(connectionString);
+      if (connStr.isZk()) {
+        this.zkHosts = List.copyOf(connStr.getQuorumItems());


no point in copying... the result of parsing isn't shared / at-risk of shared use

dsmiley · 2026-04-02T00:53:08Z

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

+import java.util.Objects;
+
+/** Universal connection string parser logic. */
+public final class CloudClientConnectionString {


Are you familiar with Java records?

Indeed, the record is more suitable

dsmiley · 2026-04-02T00:54:55Z

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

+      throw new IllegalArgumentException("Connection string must not be null or empty");
+    }
+    connectionString = connectionString.trim();
+    List<String> parts =


IMO the very first thing after trimming to do is see if "://" is found and bifercate 2 different parse methods based on that. Very simple. The ZK side would then, first thing, extract off the trailing chroot, if present, before doing comma splitting.

Just trying to suggest something simple.

Yes, it really is easier this way

dsmiley · 2026-04-02T00:57:21Z

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

+        Arrays.stream(connectionString.split(","))
+            .map(String::trim)
+            .filter(s -> !s.isEmpty())
+            .toList();


instead use org.apache.solr.common.util.StrUtils#split

I did it, but some tests failed because StrUtils#split doesn't trim spaces - I added trim() via Java stream api

dsmiley · 2026-04-02T00:58:09Z

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

+    if (parts.isEmpty()) {
+      throw new IllegalArgumentException(
+          "No valid hosts/urls found in connection string: " + connectionString);
+    }


can instead be in the constructor of the connection string

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

dsmiley · 2026-04-02T01:02:05Z

solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java

+    CloudClientConnectionString cloudClientConnectionString =
+        CloudClientConnectionString.parse(connectionString);
+    if (cloudClientConnectionString.isZk()) {
+      String zkHostNoChroot = connectionString.split("/")[0];


the chroot should be a field of cloudClientConnectionString

This is no chroot - is zkHosts joined via comma.
I can replace it by
String zkHostNoChroot = connectionString.substring(0,connectionString.length()-cloudClientConnectionString.zkChroot().length());
Or
String zkHostNoChroot = connectionString.substring(0, connectionString.length() - cloudClientConnectionString.zkChroot().length());
But IMO, currently implementation seems more pretty

cloudClientConnectionString should contain the chroot if there is one. And you just parsed it.

FWIW the name cloudClientConnectionString is terrible... it is in fact not a string but a record

dsmiley · 2026-04-02T01:11:14Z

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

    }

+    /**
+     * Provide a universal connection string that can represent either:


This method we're documenting does not "provide a connection string". Javadocs should start with what the method does and secondarily say something about the arguments.

disclaimer: I'm a stickler for javadoc style. And plenty of javadocs here are non-compliant.

Suggested change

* Provide a universal connection string that can represent either:

* Creates a client builder based on a connection string of 2 possible formats:

…nt.Builder SOLR-18130: add universal connection string support for CloudSolrClient.Builder

…on string in SolrClientCache

vvova15 · 2026-04-03T16:17:02Z

Thanks for the review!

I’ve squashed the fixes addressing your comments into the original commit.
If we decide to merge just these changes without the SolrJ-streaming refactor, I can add a changelog entry and remove the WIP label.

epugh · 2026-04-03T17:14:05Z

Maybe out of scope, but what about passing authentication info too?

dsmiley · 2026-04-03T20:58:29Z

Maybe out of scope, but what about passing authentication info too?

Can you elaborate? Exactly what/where?

dsmiley · 2026-04-03T21:01:13Z

solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java

+    CloudClientConnectionString cloudClientConnectionString =
+        CloudClientConnectionString.parse(connectionString);
+    if (cloudClientConnectionString.isZk()) {
+      String zkHostNoChroot = connectionString.split("/")[0];


cloudClientConnectionString should contain the chroot if there is one. And you just parsed it.

FWIW the name cloudClientConnectionString is terrible... it is in fact not a string but a record

dsmiley · 2026-04-03T21:03:21Z

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

+import org.apache.solr.common.util.StrUtils;
+
+/** Universal connection string parser logic. */
+public record CloudClientConnectionString(boolean isZk, List<String> quorumItems, String zkChroot) {


Lets drop the suffix String here :-)

And IMO it'd be more at home as an inner class of CloudSolrClient since it's only used for it.

dsmiley · 2026-04-03T21:05:00Z

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

+      zkHosts = connectionString.substring(0, slashIndex);
+      zkChroot = connectionString.substring(slashIndex);
+    }
+    List<String> quorumItems = StrUtils.split(zkHosts, ',').stream().map(String::trim).toList();


what test failed due to spaces within the string? IMO that'd be erroneous.

dsmiley · 2026-04-03T21:07:03Z

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

+    }
+  }
+
+  public static boolean isValidHttpUrl(String s) {


Perhaps it's a matter of taste / preference but if I were coding this, I simply wouldn't bother with this validation. The user will get an error eventually anyway if they screw up so badly as to malform their URL.

dsmiley · 2026-04-03T21:08:25Z

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

+     *       http://solr1:8983/solr,http://solr2:8983/solr}
+     * </ul>
+     *
+     * <p>Usage examples:


from this point forward, it's superfluous. The examples ought to speak for themselves.

dsmiley · 2026-04-03T21:08:48Z

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

+     * @param connectionString a string specifying either ZooKeeper connection string or HTTP(S)
+     *     Solr URLs
+     * @throws IllegalArgumentException if string is null, empty, or malformed
+     * @since 11.0.0


LOL drop the @since

dsmiley · 2026-04-03T21:09:58Z

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

+  public void testBuildQuorumForZkWithSpaces() {
+    CloudClientConnectionString parsed =
+        CloudClientConnectionString.parse(
+            " zookeeper1:2181 ,  zookeeper2:2181  , zookeeper3:2181   /solr  ");


I can't imagine someone doing this. IMO connection strings should already be free of whitespace.

By adding a test like this, you signal to everyone who looks at it -- this can happen. But really, nobody has been doing that.

github-actions bot added client:solrj tests labels Apr 1, 2026

dsmiley self-requested a review April 2, 2026 00:42

dsmiley reviewed Apr 2, 2026

View reviewed changes

vvova15 added 2 commits April 3, 2026 23:11

SOLR-18130: add universal connection string support for CloudSolrClie…

26591e1

…nt.Builder SOLR-18130: add universal connection string support for CloudSolrClient.Builder

SOLR-18130: Replace ZooKeeper solr client builder usage with connecti…

1ed3f01

…on string in SolrClientCache

vvova15 force-pushed the SOLR-18130-add-unified-connection-string-client-builder branch from 7d8cb9b to 1ed3f01 Compare April 3, 2026 16:11

dsmiley reviewed Apr 3, 2026

View reviewed changes

	* Provide a universal connection string that can represent either:
	* Creates a client builder based on a connection string of 2 possible formats:

Conversation

vvova15 commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Tests

Checklist

Uh oh!

dsmiley left a comment

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvova15 Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

vvova15 commented Apr 3, 2026

Uh oh!

epugh commented Apr 3, 2026

Uh oh!

dsmiley commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vvova15 commented Apr 1, 2026 •

edited

Loading

vvova15 Apr 3, 2026 •

edited

Loading

dsmiley commented Apr 3, 2026 •

edited

Loading