You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "jdyer1 (via GitHub)" <gi...@apache.org> on 2025/10/07 16:39:27 UTC
[PR] SOLR-17945: fix flaky test CloudHttp2SolrClientTest#testHttpCspPerf [solr]
jdyer1 opened a new pull request, #3742:
URL: https://github.com/apache/solr/pull/3742
This fixes the `CloudHttp2SolrClientTest#testHttpCspPerf` by increasing the Cluster State Provider's cache timeout.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17945: fix flaky test CloudHttp2SolrClientTest#testHttpCspPerf [solr]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3742:
URL: https://github.com/apache/solr/pull/3742#discussion_r2411237797
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -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
+ // peformance regressions!
Review Comment:
Corrected spelling of 'peformance' to 'performance'.
```suggestion
// performance regressions!
```
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -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() {
Review Comment:
Corrected spelling of 'Prperty' to 'Property' in method name.
```suggestion
public static void clearSystemProperty() {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17945: fix flaky test CloudHttp2SolrClientTest#testHttpCspPerf [solr]
Posted by "jdyer1 (via GitHub)" <gi...@apache.org>.
jdyer1 merged PR #3742:
URL: https://github.com/apache/solr/pull/3742
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17945: fix flaky test CloudHttp2SolrClientTest#testHttpCspPerf [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3742:
URL: https://github.com/apache/solr/pull/3742#discussion_r2414792451
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -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);
Review Comment:
Please don't bother clearing system properties in tests. We've got [some nice test infrastructure](https://github.com/apache/solr/blob/b74b77c212cb4574210445a3075d03cc7258cc6f/solr/test-framework/src/java/org/apache/solr/SolrTestCase.java#L92) that makes doing so needless, and here causing you to do a needless try-finally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org