From 9ba9c46cfa8421d5828c363ba8cfff1f59a1c38a Mon Sep 17 00:00:00 2001 From: jdyer1 Date: Tue, 7 Oct 2025 11:14:54 -0500 Subject: [PATCH 1/6] SOLR-17945 - create staic constant for sys prop (will cause test compile errors if removed in future) - cleanup sys prop in @AfterClass in ClusterStateProviderTest - set sys prop to very high number in testHttpCspPerf to prevent spurious CLUSTERSTATUS requests --- .../impl/BaseHttpClusterStateProvider.java | 4 +- .../solrj/impl/CloudHttp2SolrClientTest.java | 71 ++++++++++--------- .../solrj/impl/ClusterStateProviderTest.java | 9 ++- 3 files changed, 49 insertions(+), 35 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java index c6d94aaeacb..23ce0c99b37 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java @@ -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 configuredNodes; volatile Set liveNodes; // initially null then never null @@ -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 = diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java index a6d9c13e129..1bbcb115662 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java @@ -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; @@ -256,47 +257,51 @@ public void testAliasHandling() throws Exception { @Test @LogLevel("org.apache.solr.servlet.HttpSolrCall=DEBUG") public void testHttpCspPerf() throws Exception { + try { + 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")); - 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")); + SolrInputDocument doc = new SolrInputDocument("id", "1", "title_s", "my doc"); + solrClient.add(collectionName, doc); - solrClient.commit(collectionName); - // No additional admin requests sent - assertEquals(2, adminLogs.getCount()); + // 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")); - for (int i = 0; i < 3; i++) { - assertEquals( - 1, solrClient.query(collectionName, params("q", "*:*")).getResults().getNumFound()); + 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); } } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java index 18f990b41c7..eb147b21bf4 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java @@ -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; @@ -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; @@ -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 clearSystemPrperty() { + System.clearProperty(SYS_PROP_CACHE_TIMEOUT_SECONDS); } @After From 556312bbeeebce5ac950469ed5fa2cd93d95c43e Mon Sep 17 00:00:00 2001 From: jdyer1 Date: Tue, 7 Oct 2025 11:21:34 -0500 Subject: [PATCH 2/6] Add a doc comment expaining what this test is doing and why we are setting the sys prop --- .../solr/client/solrj/impl/CloudHttp2SolrClientTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java index 1bbcb115662..1f6e6b31484 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java @@ -257,8 +257,13 @@ 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 peformance 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()); From faf3e0fa2ddbf1385bdcbd5507dda2732d14998b Mon Sep 17 00:00:00 2001 From: jdyer1 Date: Tue, 7 Oct 2025 11:27:09 -0500 Subject: [PATCH 3/6] tidy --- .../solrj/impl/CloudHttp2SolrClientTest.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java index 1f6e6b31484..7235de5ed9d 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java @@ -257,20 +257,22 @@ 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 peformance regressions! + // 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 + // peformance 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. + // 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); try (LogListener adminLogs = LogListener.info(HttpSolrCall.class).substring("[admin]"); - CloudSolrClient solrClient = createHttpCSPBasedCloudSolrClient();) { + CloudSolrClient solrClient = createHttpCSPBasedCloudSolrClient(); ) { solrClient.getClusterStateProvider().getLiveNodes(); // talks to Solr assertEquals(1, adminLogs.getCount()); From e651f12577ef89e30e8d7827198fffdf49aeda0b Mon Sep 17 00:00:00 2001 From: James Dyer Date: Tue, 7 Oct 2025 11:45:16 -0500 Subject: [PATCH 4/6] Update solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../apache/solr/client/solrj/impl/ClusterStateProviderTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java index eb147b21bf4..62e2b5939eb 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java @@ -90,7 +90,7 @@ public static void setupCluster() throws Exception { } @AfterClass - public static void clearSystemPrperty() { + public static void clearSystemProperty() { System.clearProperty(SYS_PROP_CACHE_TIMEOUT_SECONDS); } From ecf1d6874b7b4ba467c7a788ffcf268eadc5df87 Mon Sep 17 00:00:00 2001 From: James Dyer Date: Tue, 7 Oct 2025 11:45:30 -0500 Subject: [PATCH 5/6] Update solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java index 7235de5ed9d..12b8588e20a 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java @@ -259,7 +259,7 @@ public void testAliasHandling() throws Exception { 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 - // peformance regressions! + // 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 From b543570f1f043d6e1dc8653e3a7af3f30fab6300 Mon Sep 17 00:00:00 2001 From: jdyer1 Date: Fri, 10 Oct 2025 09:26:11 -0500 Subject: [PATCH 6/6] Do not clear system properties. test framework does it for us. --- .../solrj/impl/CloudHttp2SolrClientTest.java | 77 +++++++++---------- .../solrj/impl/ClusterStateProviderTest.java | 6 -- 2 files changed, 37 insertions(+), 46 deletions(-) diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java index 12b8588e20a..fb721a06ea2 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java @@ -260,55 +260,52 @@ 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); + // 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); - try (LogListener adminLogs = LogListener.info(HttpSolrCall.class).substring("[admin]"); - CloudSolrClient solrClient = createHttpCSPBasedCloudSolrClient(); ) { - solrClient.getClusterStateProvider().getLiveNodes(); // talks to Solr + String collectionName = "HTTPCSPTEST"; + CollectionAdminRequest.createCollection(collectionName, "conf", 2, 1) + .process(cluster.getSolrClient()); + cluster.waitForActiveCollection(collectionName, 2, 2); - assertEquals(1, adminLogs.getCount()); - assertTrue( - adminLogs - .pollMessage() - .contains( - "path=/admin/collections params={prs=true&liveNodes=true&action" - + "=CLUSTERSTATUS&includeAll=false")); + try (LogListener adminLogs = LogListener.info(HttpSolrCall.class).substring("[admin]"); + CloudSolrClient solrClient = createHttpCSPBasedCloudSolrClient(); ) { + solrClient.getClusterStateProvider().getLiveNodes(); // talks to Solr - SolrInputDocument doc = new SolrInputDocument("id", "1", "title_s", "my doc"); - solrClient.add(collectionName, doc); + 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")); - // 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()); - solrClient.commit(collectionName); + 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()); - - 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); } } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java index 62e2b5939eb..7af3580e687 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java @@ -45,7 +45,6 @@ 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; @@ -89,11 +88,6 @@ public static void setupCluster() throws Exception { System.setProperty(SYS_PROP_CACHE_TIMEOUT_SECONDS, "1"); } - @AfterClass - public static void clearSystemProperty() { - System.clearProperty(SYS_PROP_CACHE_TIMEOUT_SECONDS); - } - @After public void cleanup() throws Exception { while (cluster.getJettySolrRunners().size() < 2) {