Skip to content

SOLR-17945: fix flaky test CloudHttp2SolrClientTest#testHttpCspPerf #3742

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 10, 2025
Merged

SOLR-17945: fix flaky test CloudHttp2SolrClientTest#testHttpCspPerf #3742

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9ba9c46
SOLR-17945
jdyer1 Oct 7, 2025
556312b
Add a doc comment expaining what this test is doing and why we are se…
jdyer1 Oct 7, 2025
faf3e0f
tidy
jdyer1 Oct 7, 2025
e651f12
Update solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterS…
jdyer1 Oct 7, 2025
ecf1d68
Update solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHtt…
jdyer1 Oct 7, 2025
b543570
Do not clear system properties. test framework does it for us.
jdyer1 Oct 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@
public abstract class BaseHttpClusterStateProvider implements ClusterStateProvider {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());

protected static final String SYS_PROP_CACHE_TIMEOUT_SECONDS = "solr.solrj.cache.timeout.sec";

private String urlScheme;
private List<URL> configuredNodes;
volatile Set<String> liveNodes; // initially null then never null
Expand All @@ -65,7 +67,7 @@ public abstract class BaseHttpClusterStateProvider implements ClusterStateProvid
long aliasesTimestamp = 0;

// the liveNodes and aliases cache will be invalidated after 5 secs
private int cacheTimeout = EnvUtils.getPropertyAsInteger("solr.solrj.cache.timeout.sec", 5);
private int cacheTimeout = EnvUtils.getPropertyAsInteger(SYS_PROP_CACHE_TIMEOUT_SECONDS, 5);

volatile boolean liveNodeReloadingScheduled = false;
private final ScheduledExecutorService liveNodeReloadingService =
Expand Down
78 changes: 45 additions & 33 deletions solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.apache.solr.client.solrj.impl;

import static org.apache.solr.client.solrj.impl.BaseHttpClusterStateProvider.SYS_PROP_CACHE_TIMEOUT_SECONDS;
import static org.apache.solr.client.solrj.impl.CloudSolrClient.RouteResponse;

import java.io.IOException;
Expand Down Expand Up @@ -256,47 +257,58 @@ public void testAliasHandling() throws Exception {
@Test
@LogLevel("org.apache.solr.servlet.HttpSolrCall=DEBUG")
public void testHttpCspPerf() throws Exception {
// This ensures CH2SC is caching cluster status by counting the number of logged calls to the
// admin endpoint. Too many calls to CLUSTERSTATUS might mean insufficient caching and
// performance regressions!
try {
// BaseHttpClusterStateProvider has a background job that pre-fetches data from CLUSTERSTATUS
// on timed intervals. This can pollute this test, so we set the interval very high to
// prevent it from running.
System.setProperty(SYS_PROP_CACHE_TIMEOUT_SECONDS, "" + Integer.MAX_VALUE);

String collectionName = "HTTPCSPTEST";
CollectionAdminRequest.createCollection(collectionName, "conf", 2, 1)
.process(cluster.getSolrClient());
cluster.waitForActiveCollection(collectionName, 2, 2);
String collectionName = "HTTPCSPTEST";
CollectionAdminRequest.createCollection(collectionName, "conf", 2, 1)
.process(cluster.getSolrClient());
cluster.waitForActiveCollection(collectionName, 2, 2);

try (LogListener adminLogs = LogListener.info(HttpSolrCall.class).substring("[admin]");
CloudSolrClient solrClient = createHttpCSPBasedCloudSolrClient(); ) {
solrClient.getClusterStateProvider().getLiveNodes(); // talks to Solr
try (LogListener adminLogs = LogListener.info(HttpSolrCall.class).substring("[admin]");
CloudSolrClient solrClient = createHttpCSPBasedCloudSolrClient(); ) {
solrClient.getClusterStateProvider().getLiveNodes(); // talks to Solr

assertEquals(1, adminLogs.getCount());
assertTrue(
adminLogs
.pollMessage()
.contains(
"path=/admin/collections params={prs=true&liveNodes=true&action"
+ "=CLUSTERSTATUS&includeAll=false"));

SolrInputDocument doc = new SolrInputDocument("id", "1", "title_s", "my doc");
solrClient.add(collectionName, doc);

// getCount seems to return a cumulative count, but add() results in only 1 additional admin
// request to fetch CLUSTERSTATUS for the collection
assertEquals(2, adminLogs.getCount());
assertTrue(
adminLogs
.pollMessage()
.contains(
"path=/admin/collections "
+ "params={prs=true&action=CLUSTERSTATUS&includeAll=false"));
assertEquals(1, adminLogs.getCount());
assertTrue(
adminLogs
.pollMessage()
.contains(
"path=/admin/collections params={prs=true&liveNodes=true&action"
+ "=CLUSTERSTATUS&includeAll=false"));

solrClient.commit(collectionName);
// No additional admin requests sent
assertEquals(2, adminLogs.getCount());
SolrInputDocument doc = new SolrInputDocument("id", "1", "title_s", "my doc");
solrClient.add(collectionName, doc);

for (int i = 0; i < 3; i++) {
assertEquals(
1, solrClient.query(collectionName, params("q", "*:*")).getResults().getNumFound());
// getCount seems to return a cumulative count, but add() results in only 1 additional admin
// request to fetch CLUSTERSTATUS for the collection
assertEquals(2, adminLogs.getCount());
assertTrue(
adminLogs
.pollMessage()
.contains(
"path=/admin/collections "
+ "params={prs=true&action=CLUSTERSTATUS&includeAll=false"));

solrClient.commit(collectionName);
// No additional admin requests sent
assertEquals(2, adminLogs.getCount());

for (int i = 0; i < 3; i++) {
assertEquals(
1, solrClient.query(collectionName, params("q", "*:*")).getResults().getNumFound());
// No additional admin requests sent
assertEquals(2, adminLogs.getCount());
}
}
} finally {
System.clearProperty(SYS_PROP_CACHE_TIMEOUT_SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't bother clearing system properties in tests. We've got some nice test infrastructure that makes doing so needless, and here causing you to do a needless try-finally.

}
}

Expand Down
9 changes: 8 additions & 1 deletion solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.solr.client.solrj.impl;

import static org.apache.solr.client.solrj.impl.BaseHttpClusterStateProvider.SYS_PROP_CACHE_TIMEOUT_SECONDS;
import static org.apache.solr.common.util.URLUtil.getNodeNameForBaseUrl;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -44,6 +45,7 @@
import org.eclipse.jetty.http.HttpHeader;
import org.hamcrest.Matchers;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import org.slf4j.Logger;
Expand Down Expand Up @@ -84,7 +86,12 @@ public static void setupCluster() throws Exception {
.resolve("conf"))
.configure();
cluster.waitForAllNodes(30);
System.setProperty("solr.solrj.cache.timeout.sec", "1");
System.setProperty(SYS_PROP_CACHE_TIMEOUT_SECONDS, "1");
}

@AfterClass
public static void clearSystemProperty() {
System.clearProperty(SYS_PROP_CACHE_TIMEOUT_SECONDS);
}

@After
Expand Down