Uh oh!
There was an error while loading. Please reload this page.
SOLR-18390: remove test-framework HTTP client convenience helpers - #4778
SOLR-18390: remove test-framework HTTP client convenience helpers#4778serhiy-bzhezytskyy wants to merge 28 commits into
Conversation
Removes the four @deprecated helpers (SolrTestCaseJ4.getHttpSolrClient overloads and JettySolrRunner.newClient()) and inlines every call site to the equivalent HttpJettySolrClient.Builder construction. Test-only change, no behavior change.
The mechanical migration called .getLeader() separately for getBaseUrl() and getCoreName(). For PRS collections getLeader() recomputes the leader on every call (its own comment: 'leader may keep changing') -- calling it twice risked getBaseUrl() and getCoreName() coming from two different replicas if the leader changed in between. Capture it once instead.
9ea76db to
92ed892Compareepugh
commented
Aug 22, 2026
@dsmiley I think we need your eyes here. I thought this was going to be a more "oh wow, that is so much nicer" type of impact when I looked at the changelog. More "oh yes, the tests are more readable, more compact" and I didn't get that. However, didn't you have some thoughts about providing the client via TestRule or other ideas that maybe would be a fit? |
…newClient() call sites CI (ecjLintTest / compileTestJava) failed: the solr/webapp module wasn't covered by the original migration sweep, so its 8 call sites across 5 test files still called the now-removed no-arg newClient(). Migrated them to the same new HttpJettySolrClient.Builder(baseUrl).build() pattern used everywhere else in this PR.
serhiy-bzhezytskyy
commented
Aug 22, 2026
@epugh could you add the |
dsmiley
commented
Aug 23, 2026
I should have expressed more when I deprecated that method recently; sorry. I deprecated it because I'd guess that most callers should call getSolrClient() and then don't close it either, as it's lifecycle is managed by the runner. That loses a try-finally on the caller (nice), and avoids needless work and additional connections (nice). The clients are immutable (or should be). So why create a new vanilla client when the JettySolrRunner already has one? Additionally there's a new method: |
…eplicas() call solr:webapp wasn't covered by this PR's own migration sweep -- same recurring gap as SOLR-18390/apache#4778 and SOLR-18357/apache#4790. replicaCount() still called the now-removed getReplicas().size(); migrated to replicaStream().count() (cast to int, matching the method's return type), the same pattern used at every other call site in this PR.
…a new one Where a JettySolrRunner reference is directly available, use its own getSolrClient()/newSolrClient(collection) instead of building a fresh HttpJettySolrClient. 59 call sites (51 core/benchmark, 8 webapp). newSolrClient() builds a fresh, uncached client per call (unlike getSolrClient()), so those sites keep an explicit close -- caught via ObjectReleaseTracker flagging 9 leaked clients before this fix. 3 sites left as constructed clients for verified reasons: explicit close ownership elsewhere, a stop()/start() cycle invalidating a cached client mid-test, or an in-flight request needing to survive a runner shutdown that closes the cached client first.
Removed the 2-arg timeout overload too, per the reviewer's follow-up: he'd rather not keep it either, and it has few callers. Migrated the 4 callers to the shared getSolrClient() rather than reconstructing a custom-timeout client: their explicit timeouts (15s conn/60s idle, 30s/300s) are all shorter than the client's own default (60s/600s), so nothing was relying on them -- the shared client's default gives at least as much headroom either way.
…tion
No variable-name collision in the enclosing method required scoping
this off from a try(...) block; de-indented instead of wrapping in a
pointless bare {}.…JettyRunner.jetty shardToJetty.get(SHARD2).get(0) is a CloudJettyRunner, which carries a public JettySolrRunner jetty field right next to the Replica-shaped info map the code was pulling a raw base_url string from. Same fix as the other 59 sites, just reached through a different field on the same object instead of a direct JettySolrRunner variable.
…ibZkTestBase Experiment: this test never used any AbstractFullDistribZkTestBase-specific machinery (ChaosMonkey, control_collection, shard-consistency checks) -- setupRestTestHarnesses()/randomRestTestHarness() already delegated to JettySolrRunner.getRestClient() internally, so the only real work was replicating the implicit cluster/collection setup explicitly on SolrCloudTestCase. Passed first try. Not a template for a blanket migration -- most subclasses of the legacy base actually use the machinery this one didn't.
serhiy-bzhezytskyy
commented
Aug 23, 2026
Reworked to reuse getSolrClient()/newSolrClient() where a runner reference was available. Also removed newClient(int, int) -- its 4 callers all used timeouts shorter than the shared client's own default, so nothing needed them. Also tried migrating one AbstractFullDistribZkTestBase test (TestCustomStream) to SolrCloudTestCase as an experiment -- worked first try for this trivial case, but required tracing 2 layers of legacy inheritance to find the implicit setup defaults. Worth a separate, incremental effort, not a blanket migration. |
…t-http-client-helpers
…t-http-client-helpers # Conflicts: # solr/core/src/test/org/apache/solr/cloud/TestPullReplica.java
| solrCluster.uploadConfigSet(configset("configset-2"), collectionName); | ||
| final String baseUrl = solrCluster.getJettySolrRunners().get(0).getBaseUrl().toString(); | ||
| final SolrClient solrClient = getHttpSolrClient(baseUrl); | ||
| final SolrClient solrClient = new HttpJettySolrClient.Builder(baseUrl).build(); |
There was a problem hiding this comment.
can get a new client via solrCluster.getJettySolrRunners().get(0).newSolrClient(). Please apply this pattern wherever possible on tests in this PR -- where the test code deliberately needs a new client for some reason, and a JettySolrRunner is in-scope.
There was a problem hiding this comment.
Done — applied everywhere a runner was in scope. Two helper methods took baseUrl as a bare String param (SolrJMetricTestUtils.getNumNodeRequestErrors, BasicDistributedZk2Test.addAndQueryDocs); changed both to take JettySolrRunner since their only callers already held one. Also removed BasicDistributedZkTest.createSolrCore, a private method with zero callers anywhere in the repo, found during the sweep.
Replaces manual HttpJettySolrClient.Builder(...) construction with JettySolrRunner.newSolrClient() across this PR's test changes, per David's review comment. Also removes BasicDistributedZkTest.createSolrCore, a private method with zero callers found during the sweep.
dsmiley
left a comment
There was a problem hiding this comment.
This is a biggie... we've started on this one but I wish we could moderate/influence which issues you take up before you start as it's a large review undertaking. For this one in particular, there is an overarching theme of SolrClient implementations already existing and managed by JettySolrRunner which one can get one way or another usually. Couple that with CollectionScopedSolrClient and we can use it even if we need to talk to a specific core/replica/collection/alias. There ought to be more conveniences in-place to make this a bit easier. I added some review comments hoping you might take some of them up.
| // pick an arbitrary node to use for our requests | ||
| client = cluster.getRandomJetty(random()).newClient(); | ||
| client = | ||
| new HttpJettySolrClient.Builder(cluster.getRandomJetty(random()).getBaseUrl().toString()) |
There was a problem hiding this comment.
can be newSolrClient on the jetty
There was a problem hiding this comment.
Done — cluster.getRandomJetty(random()).newSolrClient(null).
| try (SolrClient coreClient = | ||
| getHttpSolrClient(cluster.getZkStateReader().getBaseUrlForNodeName(nodeName))) { | ||
| new HttpJettySolrClient.Builder(cluster.getZkStateReader().getBaseUrlForNodeName(nodeName)) |
There was a problem hiding this comment.
Can I convince you to add MiniSolrCloudCluster.getJetty(nodeNameOrUrl)? Then you'd call it here with getSolrClient(); no need to close.
There was a problem hiding this comment.
Added MiniSolrCloudCluster.getJetty(nodeNameOrUrl) and switched to it. Turned out nodeName in this test is actually nodeName/coreNodeName (the success-map key, per CollectionHandlingUtils.requestKey), so I split that off before the lookup.
| String coreName = replica.getCoreName(); | ||
| try (SolrClient client = getHttpSolrClient(replica)) { | ||
| try (SolrClient client = |
There was a problem hiding this comment.
Can I convince you to add a MiniSolrCloudCluster.getSolrClient(Replica) method that lookups up the right Jetty (see getReplicaJetty)... and then calls getSolrClient() on it and then wraps that with CollectionScopedSolrClient for the core?
There was a problem hiding this comment.
Added MiniSolrCloudCluster.getSolrClient(Replica), wrapping getReplicaJetty(replica).getSolrClient() with CollectionScopedSolrClient, and switched the call site to it.
Use newSolrClient() where a JettySolrRunner is already in scope, and add MiniSolrCloudCluster.getJetty(nodeNameOrUrl) / getSolrClient(Replica) so the other two tests can reuse a jetty's shared client instead of opening and closing their own.
…t-http-client-helpers # Conflicts: # solr/core/src/test/org/apache/solr/cloud/DistribDocExpirationUpdateProcessorTest.java # solr/core/src/test/org/apache/solr/cloud/MigrateRouteKeyTest.java # solr/core/src/test/org/apache/solr/cloud/NestedShardedAtomicUpdateTest.java
…t-http-client-helpers # Conflicts: # solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java # solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java
dsmiley
left a comment
There was a problem hiding this comment.
Love the new utility methods :-)
In many of the various classes (I didn't point out each case), you can use the new methods you added (especially the Replica one) to get an existing client
There was a problem hiding this comment.
several spots here can use the new method you added to get an existing client by Replica on the cluster
There was a problem hiding this comment.
can use the new method you added to get an existing client by Replica
There was a problem hiding this comment.
can use the new method you added to get an existing client by Replica
There was a problem hiding this comment.
can use the new method you added to get an existing client by Replica
There was a problem hiding this comment.
can use the new method you added to get an existing client by Replica
…scope Replaces 34 hand-built clients across 14 test classes. Since the returned client is cluster-owned, try-with-resources and explicit close() calls on them are dropped.
…ites getReplicaJetty now also matches a jetty's proxy URL, so it resolves in tests that front a node with a SocketProxy. The same lookup is shared with AbstractFullDistribZkTestBase, which gains its own getSolrClient(Replica) since it has jettys but no MiniSolrCloudCluster.
serhiy-bzhezytskyy
commented
Aug 29, 2026
Done |
Each needed a different route: node-level CoreAdmin calls take the jetty's own client, a core-scoped query takes getSolrClient(Replica), a node named by nodeName takes getJetty(name), and the two _stateVer_ cases wrap the node's client in CollectionScopedSolrClient to keep addressing by collection.
MigrateReplicasTest/ReplaceNodeTest/TestCoordinatorRole address a node by name or URL, so they take getJetty(...).getSolrClient(). The two tests that keep a set of per-node collection-scoped clients now wrap the nodes' own clients, so their @afterclass no longer closes them.
… one TestPrometheusResponseWriter's six metrics calls and one of the bad-input cases are node-level, so they use getAdminClient(); the collection-scoped bad-input case uses getSolrClient(collection).
Two tests that build one client per runner now wrap the runner's own client, so their @afterclass no longer closes them. UpdateLogCloudTest keeps its own because it restarts a node, which the cached client does not survive.
…tty in lookups AbstractFullDistribZkTestBase keeps controlJetty outside jettys, so a replica hosted there was unresolvable -- ShardSplitTest only surfaced it under -Ptests.nightly=true. Both lookups now search every runner.
The wrapper's default collection is unused by CoreAdmin requests, so the method David asked for works there as well and keeps one route instead of two.
…ica) The earlier attempt failed only because controlJetty sat outside the lookup; with that fixed the conversion works. Every converted site runs with the proxies reopened, or in the ZK-expiry test where no proxy is involved, so nothing loses the partition it was exercising.
https://issues.apache.org/jira/browse/SOLR-18390
Removes the four
@Deprecatedtest-framework HTTP client convenience helpers (SolrTestCaseJ4.getHttpSolrClientoverloads andJettySolrRunner.newClient()) and inlines every call site to the equivalentHttpJettySolrClient.Builderconstruction -- matching what the deprecated methods' own bodies did. Test-only change, no changelog.84 files, 173 call sites migrated. While migrating, found and fixed a subtle bug the mechanical inlining would otherwise have introduced: several sites called
Slice.getLeader()twice (once for the URL, once for the collection name). For PRS collectionsgetLeader()recomputes the leader on every call, so calling it twice could return two different replicas if the leader changed in between. Fixed by capturing it once.Ran the highest call-site-density test classes plus everything touched by the getLeader() fix, all green (0 failures): FullSolrCloudDistribCmdsTest, AliasIntegrationTest, TestCoordinatorRole, LB2SolrClientTest, TestPullReplica (nightly), TestPullReplicaErrorHandling, TestPullReplicaWithAuth, TestTlogReplica, HealthCheckHandlerTest, MultiAuthPluginTest, BasicAuthIntegrationTest, HttpSolrClientBadInputTest, TestJettySolrRunner, BasicDistributedZkTest, BasicDistributedZk2Test.
AI-assisted (Claude Sonnet 5)