SOLR-18380: remove CloudSolrClient.connect(), ClusterStateProvider.connect() and the legacy Builder by serhiy-bzhezytskyy · Pull Request #4762 · apache/solr

serhiy-bzhezytskyy · 2026-08-19T15:23:05Z

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

Removes CloudSolrClient.connect()/connect(long, TimeUnit), ClusterStateProvider.connect(), and the legacy Builder(List<String>, Optional<String>) — 54 call sites. connect() was never substantive in any implementation (one was literally getLiveNodes();); callers now call getClusterStateProvider().getLiveNodes() directly. getClusterState() is left alone, per the ticket's own note that its future is undecided.

Where to look: the Builder(List, Optional) → Builder(String) swap is not a pure rename — the connection-string parser takes everything after the first / as the chroot, which the two-arg form discarded. RunExampleTool picks up a real fix from this (its zkHost can carry a chroot).

Two risks no compiler would catch, both fixed: a mock returning null from getClusterStateProvider() turned a passing test into a 150s hang instead of a failure (SolrMessageProcessorTest, now stubs the provider); and a broken var initializer on one line silently swallowed the compile error on the next (TestSimplePropagatorDistributedTracing).

54 call sites, 19 changed test classes (one @ignore'd abstract base correctly has no tests of its own — its 3 concrete subclasses do), compile + gates clean.

SOLR-18373, SOLR-18378, SOLR-18382 and SOLR-18386 touch files this PR also touches — merging this one first should make those cleaner to extract.

AI-assisted (Claude Sonnet 5)

@ignore

…nnect() and the legacy Builder Four deprecated members, 54 call sites found over five compile rounds. connect() was never substantive, and each implementation says so in its own words: BaseHttpClusterStateProvider.connect() was literally "getLiveNodes();", ZkClientClusterStateProvider.connect() was commented "Essentially a No-Op, but force a check that we're not closed", and DelegatingClusterStateProvider.connect() delegated. getLiveNodes() on the ZK provider goes through the same getZkStateReader(), so it throws AlreadyClosedException in exactly the same situations. Callers that wanted a forced connection now call getClusterStateProvider().getLiveNodes(). connect(long, TimeUnit) was the one substantive member - a 250 ms poll to a timeout. It had no production caller. The equivalent already exists in production as ConnectionImpl.connect(CloudSolrClient, long, TimeUnit) in solrj-streaming, built on getLiveNodes() in a timeout loop, if anyone needs it back. Builder(List, Optional) becomes Builder(String). This is not a pure rename: the connection-string parser takes the chroot to be everything from the first '/', while the old two-arg form put the whole string in the host list and dropped the chroot. In RunExampleTool the zkHost value comes from the -zkHost option or a running node, whose standard Solr form is host:2181/solr, so the migration there also fixes a latent bug. That is called out in a comment at the site. getClusterState() is left alone, per the ticket: its own javadoc says the future of ClusterState isn't clear, so it is scoped out of this removal. Three things that no compiler would have caught. A void method on a mock is a silent success; a chained one is a silent NPE. SolrMessageProcessor.connectToSolrIfNeeded is an unbounded while loop that sleeps 5 s on any exception. With the old client.connect() a bare mock returned harmlessly and the loop exited on the first pass. After the migration, getClusterStateProvider() returns null on the mock, the NPE is caught, and the test hangs instead of failing. Measured: 3.3 s with a stubbed provider, killed at 150 s without one. Fixed by stubbing the provider in setUp and verifying getLiveNodes() on it, which keeps the only coverage of that loop being entered exactly once. `var` truncates the compiler's own worklist. In TestSimplePropagatorDistributedTracing a failing `var builder = ...` made `var client = builder.build()` erroneous too, so the client.connect() on the next line was never reported. After fixing a var initialiser, the rest of that method body has to be re-read by hand. RoutedAliasUpdateProcessorTest is an @ignore'd abstract base with no tests of its own; its three concrete subclasses (Category/Dimensional/TimeRoutedAliasUpdateProcessorTest) carry the coverage and are what was run. Verified: compileJava and compileTestJava across solrj, solrj-zookeeper, solrj-streaming, core, test-framework and cross-dc-manager, spotlessCheck, ecjLintMain/Test on every touched module, renderJavadoc on solrj and core, and 19 changed test classes (counting the three RoutedAliasUpdateProcessorTest subclasses, not the abstract base) - 110 tests, 0 failures. AI-assisted (Claude Sonnet 5)

serhiy-bzhezytskyy · 2026-08-19T19:10:37Z

@dsmiley you deprecated all of this two months ago (CloudSolrClient.connect() x2, the legacy Builder, ClusterStateProvider.connect()), so it's fresh enough to actually need your take. 54 sites across 5 modules, and migrating them turned up two places where the removal could've silently broken a test (a mock returning null -> 150s hang; a broken var swallowing a compile error) -- fixed, but a second pair of eyes from whoever wrote the deprecation would be reassuring.

AI-assisted (Claude Sonnet 5)

…udsolrclient-connect

dsmiley

Thanks for this!
Seems the scope is more than connect().

Two risks no compiler would catch, both fixed: a mock returning null from getClusterStateProvider() turned a passing test into a 150s hang instead of a failure (SolrMessageProcessorTest, now stubs the provider);

This is a standard risk of mocking frameworks and a reason why I personally strongly avoid them.

and a broken var initializer on one line silently swallowed the compile error on the next (TestSimplePropagatorDistributedTracing).

I didn't see this

dsmiley · 2026-08-19T23:38:26Z

  protected void waitToSeeLiveNodes(String zkHost, int numNodes) {
-    try (CloudSolrClient cloudClient =
-        new CloudSolrClient.Builder(List.of(zkHost), Optional.empty()).build()) {
+    // honours a chroot inside zkHost, e.g. zk1:2181/solr


nice comment

I happen to like the spelling as well ;-). I like to paint my armour using colours ;-)

dsmiley · 2026-08-19T23:39:18Z

  private void clusterStatusRolesTest() throws Exception {
    try (CloudSolrClient client = createCloudClient(null)) {
-      client.connect();
+      client.getClusterStateProvider().getLiveNodes(); // force the connection now


Unnecessary, I presume

Confirmed -- CloudSolrClient.sendRequest() unconditionally calls provider.getLiveNodes() for a request without a collection param, which is what client.request(...) sends a few lines later here (and getClusterState() in the other spot delegates to the same lazy-init path). So the forcing call was dead weight, not a migration bug -- removed in all 4 spots you flagged, both test classes re-run green.

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-19T23:39:24Z

  private void replicaPropTest() throws Exception {
    try (CloudSolrClient client = createCloudClient(null)) {
-      client.connect();
+      client.getClusterStateProvider().getLiveNodes(); // force the connection now


Unnecessary, I presume

Same as the TestCollectionAPI.java:690 thread -- removed.

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-19T23:39:34Z

  private void testShardCreationNameValidation() throws Exception {
    try (CloudSolrClient client = createCloudClient(null)) {
-      client.connect();
+      client.getClusterStateProvider().getLiveNodes(); // force the connection now


Unnecessary, I presume

Same as the TestCollectionAPI.java:690 thread -- removed.

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-19T23:39:53Z


    try (CloudSolrClient client = createCloudClient(null)) {
-      client.connect();
+      client.getClusterStateProvider().getLiveNodes(); // force the connection now


Unnecessary, I presume

Same as the TestCollectionAPI.java:690 thread -- removed.

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-19T23:51:31Z

dsmiley · 2026-08-19T23:52:46Z


      CloudSolrClient solrClient = cluster.getSolrClient();
-      solrClient.connect();
+      solrClient.getClusterStateProvider().getLiveNodes(); // force the connection now


Hmm; I suspect this isn't necessary. I believe obtaining the ZkStateReader itself implies a connection.

Confirmed -- removed. ZkStateReader.from(solrClient) on the very next line forces the same lazy connection, same as the 4 spots you flagged earlier.

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-20T00:01:15Z

    cloudInit = true;
    cloudClient = createCloudClient(DEFAULT_COLLECTION);
-    cloudClient.connect();
+    cloudClient.getClusterStateProvider().getLiveNodes(); // force the connection now


ZkStateReader.from will do that

Right -- ZkStateReader.from(cloudClient) two lines below does exactly that. Removed.

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-20T00:03:07Z

+    // force the ZkStateReader into existence
+    if (reader == null) cluster.getSolrClient().getClusterStateProvider().getLiveNodes();


getZkStateReader will do that well enough

Agreed -- simplified to cluster.getZkStateReader().getZkClient(). Verified getZkStateReader() never returns null: 4000 concurrent calls right after cluster startup, all non-null, and a post-shutdown call throws AlreadyClosedException, never null. Dug into why the null-check existed: in 2017 (SOLR-10278) getZkStateReader() was a bare field getter, nullable before first connection -- the guard was correct then. SOLR-13393 (2019-04-11) rewrote it into today's lazy-init-or-throw pattern to fix a ZkStateReader leak on shutdown, closing the null path for good. The guard's been unreachable ever since -- over 6 years.

AI-assisted (Claude Sonnet 5)

dsmiley · 2026-08-20T00:05:24Z

  }

  private CloudSolrClient newCloudSolrClient() {
-    var builder =


the PR intro implied this var isn't what we think it is. Isn't it a builder?

Yes, builder is a CloudSolrClient.Builder -- unambiguous at the source level. The PR description was describing a build-time artifact: reintroduced both removed calls together and confirmed javac reports only the constructor error, silently swallowing the separate client.connect() removal error on the next line (its type can't resolve once builder's initializer fails). Not a type-identity issue, just an ambiguous sentence on my part -- sorry.

AI-assisted (Claude Sonnet 5)

…ests CloudSolrClient.sendRequest() already calls provider.getLiveNodes() unconditionally for a collection-less request, and getClusterState() delegates to the same lazy-init path -- so the migrated connect() replacement was dead weight in these 4 spots, not a migration bug.

…7 call sites Replaces the hand-rolled `new CloudSolrClient.Builder(cluster.getZkServer() .getZkAddress())` pattern, repeated across 5 test classes, with a single builder method on the cluster. One more site (ForceLeaderTest) uses the older AbstractFullDistribZkTestBase framework with its own `zkServer` field, not MiniSolrCloudCluster -- left untouched, out of scope for this method.

The comment described the replacement line by comparing it to a method this same PR deletes -- reworded to describe what the line does instead.

JdbcTest and AbstractFullDistribZkTestBase both call ZkStateReader.from() on the very next line, which forces the same lazy connection -- the explicit call was redundant, same shape as the earlier 4-site fix. SolrCloudTestCase.zkClient()'s null-check was dead code: getZkStateReader() never returns null (verified empirically, 4000 concurrent calls plus a post-shutdown call that throws AlreadyClosedException instead). Git archaeology on ZkClientClusterStateProvider confirms why: it was a bare, genuinely-nullable field getter in 2017 (SOLR-10278), and SOLR-13393 (2019) rewrote it into today's lazy-init-or-throw form to fix a ZkStateReader leak on shutdown -- the guard has been unreachable ever since, its protection superseded by that fix.

dsmiley · 2026-08-20T13:35:39Z

Just to share that I sometimes lean in favor of a CHANGELOG -- I think this is a good one CC @janhoy so you notice ;-)

dsmiley · 2026-08-20T13:37:58Z

+   * cluster's ZooKeeper ensemble, to customize further (default collection, update routing, HTTP
+   * client, ...) before calling {@code build()}.
+   */
+  public CloudSolrClient.Builder getSolrClientBuilder() {


rename to newSolrClientBuilder; we aren't returning something we already have.

dsmiley · 2026-08-20T13:38:52Z

+   * Returns a new {@link org.apache.solr.client.solrj.impl.CloudSolrClient.Builder} pointed at this
+   * cluster's ZooKeeper ensemble, to customize further (default collection, update routing, HTTP


don't overdocument implementations details (AI does this all the time). This is an implementation detail that I plan to change to pick randomly between the ZK & HTTP ClusterState at some point.

dsmiley · 2026-08-20T13:45:17Z


  private void assertCollectionExists(String name) {
-    solrClient.getClusterStateProvider().connect(); // TODO get rid of this
+    solrClient.getClusterStateProvider().getLiveNodes(); // TODO get rid of this


Your comment/investigation #4762 (comment) nicely confirms you can remove this

github-actions Bot added documentation Improvements or additions to documentation test-framework client:solrj tests cat:cloud cat:index cat:cli labels Aug 19, 2026

This was referenced Aug 19, 2026

SOLR-18378: remove CoreContainer.getCores() and SolrCores.getCores(), 26 sites #4764

Open

SOLR-18386: remove the deprecated Utils.getBaseUrlForNodeName overloads #4766

Open

Merge remote-tracking branch 'origin/main' into SOLR-18380-remove-clo…

2cd81db

…udsolrclient-connect

dsmiley reviewed Aug 20, 2026

View reviewed changes

serhiy-bzhezytskyy added 4 commits August 20, 2026 12:22

SOLR-18380: stop naming the removed connect() method in a comment

096eaa1

The comment described the replacement line by comparing it to a method this same PR deletes -- reworded to describe what the line does instead.

dsmiley self-requested a review August 20, 2026 13:34

dsmiley reviewed Aug 20, 2026

View reviewed changes

		// force the ZkStateReader into existence
		if (reader == null) cluster.getSolrClient().getClusterStateProvider().getLiveNodes();

		* Returns a new {@link org.apache.solr.client.solrj.impl.CloudSolrClient.Builder} pointed at this
		* cluster's ZooKeeper ensemble, to customize further (default collection, update routing, HTTP

Uh oh!

Conversation

serhiy-bzhezytskyy commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-bzhezytskyy commented Aug 19, 2026

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!

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!

serhiy-bzhezytskyy Aug 20, 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!

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

serhiy-bzhezytskyy commented Aug 19, 2026 •

edited

Loading

serhiy-bzhezytskyy Aug 20, 2026 •

edited

Loading