Skip to content

SOLR-18188: Remove org.apache.solr.client.solrj.apache; update tests to use Jetty equivalents#4451

Draft
dsmiley wants to merge 32 commits into
apache:mainfrom
dsmiley:SOLR-18188-testsStopUsingApacheHttpClient
Draft

SOLR-18188: Remove org.apache.solr.client.solrj.apache; update tests to use Jetty equivalents#4451
dsmiley wants to merge 32 commits into
apache:mainfrom
dsmiley:SOLR-18188-testsStopUsingApacheHttpClient

Conversation

@dsmiley
Copy link
Copy Markdown
Contributor

@dsmiley dsmiley commented May 19, 2026

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

This branch/PR originated on the HttpSolrClient refactor / re-addition, whcih is related. That aspect has since merged; brought this branch up to date. I'll want to take chunks of work here and commit separately (other PRs) so we don't lose context on why some tests or test infra changed in non-obvious ways.

As of this writing: tests pass, even with SSL. "nightly" tests have failures.

Not going to be a good use of time to review this massive thing so I suggest not reviewing except for the things I call out in a self-review.

dsmiley and others added 29 commits October 31, 2025 22:28
WILL NOT COMPILE

Not widely updating anything yet; just touch this source file.
WILL NOT COMPILE

Enhanced our clients to use it.
# Conflicts:
#	solr/core/src/test/org/apache/solr/cloud/HttpPartitionTest.java
#	solr/core/src/test/org/apache/solr/cloud/LeaderTragicEventTest.java
#	solr/core/src/test/org/apache/solr/cloud/LeaderVoteWaitTimeoutTest.java
#	solr/core/src/test/org/apache/solr/cloud/RecoveryZkTestWithAuth.java
#	solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java
#	solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java
#	solr/core/src/test/org/apache/solr/cloud/TestRandomRequestDistribution.java
#	solr/core/src/test/org/apache/solr/filestore/TestDistribFileStore.java
#	solr/core/src/test/org/apache/solr/handler/admin/CoreAdminHandlerTest.java
#	solr/core/src/test/org/apache/solr/handler/admin/ZookeeperStatusHandlerFailureTest.java
#	solr/core/src/test/org/apache/solr/handler/admin/ZookeeperStatusHandlerTest.java
#	solr/core/src/test/org/apache/solr/handler/component/DistributedQueryElevationComponentTest.java
#	solr/core/src/test/org/apache/solr/pkg/TestPackages.java
#	solr/core/src/test/org/apache/solr/search/TestCoordinatorRole.java
#	solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleBinaryTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleCborTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleXMLTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/SolrExceptionTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/SolrSchemalessExampleTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/TestSolrJErrorHandling.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/response/InputStreamResponseParserTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/response/TestSuggesterResponse.java
#	solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
#	solr/test-framework/src/java/org/apache/solr/client/solrj/apache/ConcurrentUpdateSolrClient.java
#	solr/test-framework/src/java/org/apache/solr/client/solrj/apache/HttpApacheSolrClient.java
#	solr/test-framework/src/java/org/apache/solr/client/solrj/apache/SolrClientBuilder.java
#	solr/test-framework/src/java/org/apache/solr/cloud/AbstractBasicDistributedZkTestBase.java
#	solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java
#	solr/test-framework/src/java/org/apache/solr/cloud/AbstractRecoveryZkTestBase.java
#	solr/test-framework/src/java/org/apache/solr/cloud/AbstractSyncSliceTestBase.java
#	solr/test-framework/src/java/org/apache/solr/cloud/AbstractUnloadDistributedZkTestBase.java
#	solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java
#	solr/test-framework/src/test/org/apache/solr/client/solrj/apache/BasicHttpSolrClientTest.java
#	solr/test-framework/src/test/org/apache/solr/client/solrj/apache/HttpSolrClientBuilderTest.java
Rename HttpSolrClientBuilderBase.java to HttpSolrClient.BuilderBase (inner class)
# Conflicts:
#	solr/solr-ref-guide/modules/indexing-guide/pages/indexing-with-tika.adoc
…ient

# Conflicts:
#	solr/core/src/test/org/apache/solr/handler/TestReplicationHandlerBackup.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/TestBatchUpdate.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/TestSolrJErrorHandling.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBadInputTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/response/InputStreamResponseParserTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/response/TestSuggesterResponse.java
#	solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java
#	solr/test-framework/src/test/org/apache/solr/client/solrj/apache/BasicHttpSolrClientTest.java
# Conflicts:
#	solr/core/src/test/org/apache/solr/cloud/TestTlogReplica.java
#	solr/core/src/test/org/apache/solr/filestore/TestDistribFileStore.java
#	solr/core/src/test/org/apache/solr/handler/admin/api/ClusterPropsAPITest.java
#	solr/core/src/test/org/apache/solr/metrics/SolrMetricsIntegrationTest.java
#	solr/core/src/test/org/apache/solr/pkg/TestPackages.java
#	solr/core/src/test/org/apache/solr/search/TestCoordinatorRole.java
#	solr/core/src/test/org/apache/solr/servlet/CacheHeaderTestBase.java
#	solr/core/src/test/org/apache/solr/servlet/SecurityHeadersTest.java
#	solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java
#	solr/solrj-streaming/gradle.lockfile
#	solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleBinaryTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleCborTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleXMLTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/SolrExceptionTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/SolrSchemalessExampleTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/TestLBHttpSolrClient.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java
#	solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
#	solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java
#	solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java
#	solr/test-framework/src/java/org/apache/solr/util/RestTestBase.java
#	solr/test-framework/src/test/org/apache/solr/client/solrj/apache/BasicHttpSolrClientTest.java
#	solr/test-framework/src/test/org/apache/solr/client/solrj/apache/ConnectionReuseTest.java
#	solr/test-framework/src/test/org/apache/solr/client/solrj/apache/HttpSolrClientBuilderTest.java
#	solr/test-framework/src/test/org/apache/solr/client/solrj/apache/HttpSolrClientConPoolTest.java
#	solr/test-framework/src/test/org/apache/solr/client/solrj/apache/LBHttpSolrClientTest.java
# Conflicts:
#	changelog/unreleased/SOLR-17968-HttpSolrClient.yml
#	solr/solrj/src/java/org/apache/solr/client/solrj/impl/CollectionScopedSolrClient.java
#	solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java
#	solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java
#	solr/test-framework/src/java/org/apache/solr/client/solrj/apache/HttpApacheSolrClient.java
The race condition was that chaosMonkey.expireSession() starts a background
coreZkRegister thread, but waitForRecoveriesToFinish() returned immediately
because the node hadn't re-joined live_nodes yet (non-live replicas in DOWN
state are ignored). The fix increases blockUntilConnected from 50ms to 30s
and replaces waitForRecoveriesToFinish with a waitForState that explicitly
waits for the expired node to be back in live_nodes and all live replicas
to be ACTIVE.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label May 19, 2026
@github-actions github-actions Bot removed the admin-ui label May 20, 2026
testOptions += [
[propName: 'tests.src.home', value: null, description: "See SOLR-14023."],
[propName: 'solr.tests.use.numeric.points', value: null, description: "Point implementation to use (true=numerics, false=trie)."],
[propName: 'tests.ssl', value: false, description: "Force SSL on for all tests that support it (respects @SuppressSSL)."],
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create a separate PR for this build convenience thing. It's also debatable it should be elevated to be in this list.

@@ -1,188 +0,0 @@
/*
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't a test, it's really some benchmark-like thing from ancient times that never should have been merged IMO

import org.junit.Test;
import org.mockito.Mockito;

@LuceneTestCase.AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-17810")
Copy link
Copy Markdown
Contributor Author

@dsmiley dsmiley May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I told the contributor of SOLR-17810 (not quite ready to merge) that this is needed

request.setResponseParser(new InputStreamResponseParser(wt));
}
result = client.request(request, testCollection);
InputStream inputStream = (InputStream) result.get("stream");
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done in several tests because we must close the stream now. Previously we could get away with not doing so.

*/
@SuppressWarnings("unchecked")
public B withHttpClient(C httpSolrClient) {
if (this.baseSolrUrl == null) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Belongs in another PR.


/** Apache HttpClient based {@link org.apache.solr.client.solrj.SolrClient} implementations */
@Deprecated
package org.apache.solr.client.solrj.apache;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this whole package :-) bye bye HttpSolrClient, CloudLegacySolrClient, etc.

IOUtils.closeQuietly(jettySolrClient);
jettySolrClient = null;

if (enableProxy) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved; related to a tricky test fix that's kinda related to the transition

* A variant of {@link org.apache.solr.client.solrj.impl.CloudSolrClient.Builder} that will
* randomize some internal settings.
*/
public static class RandomizingCloudHttp2SolrClientBuilder extends CloudSolrClient.Builder {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically I renamed the http2 one to RandomizingCloudSolrClientBuilder


protected volatile JettySolrRunner controlJetty;
protected final List<SolrClient> clients = Collections.synchronizedList(new ArrayList<>());
protected final List<HttpSolrClient> clients = Collections.synchronizedList(new ArrayList<>());
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using this more specifc type allows getBaseURL

.setCreateNodeSet(controlJetty.getNodeName())
.process(client)
.getStatus());
RetryUtil.retryOnException(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love the retry but it's needed sometimes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant