You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "epugh (via GitHub)" <gi...@apache.org> on 2026/01/05 10:55:53 UTC
[PR] Remove unneeded clearing of system properties [solr]
epugh opened a new pull request, #4010:
URL: https://github.com/apache/solr/pull/4010
# Description
Test infrastructure clears properties after a test is run, therefore these lines of code are not needed.
# Solution
Remove system properties that are cleared in `@After` and `@AfterClass` annotated methods.
# Tests
Re-run the tests. Also investigating the use of the `-Ptests.multiplier=8` and `-Ptests.nightly=true` parameters to stress test.
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2672748709
##########
solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java:
##########
@@ -59,6 +59,7 @@ public void setupCluster() throws Exception {
System.setProperty("solr.directoryFactory", "solr.StandardDirectoryFactory");
System.setProperty("solr.ulog.numRecordsToKeep", "1000");
System.setProperty("leaderVoteWait", "60000");
+ System.setProperty("tests.zk.violationReportAction", "IGNORE");
Review Comment:
That is a fair comment... Theoretically, since this PR is about removing unneeded clearing of properties, then any improvements to the tests shouldn't be in this PR...
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2672753884
##########
solr/core/src/test/org/apache/solr/core/DirectoryFactoryTest.java:
##########
@@ -48,7 +47,6 @@ public static void cleanupLoader() throws Exception {
loader = null;
}
- @After
@Before
public void clean() {
System.clearProperty("solr.data.home");
Review Comment:
You got this one flipped! We remove the after, but not the before ;-)
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2672765304
##########
solr/core/src/test/org/apache/solr/handler/component/InfixSuggestersTest.java:
##########
@@ -44,6 +44,10 @@ public static void beforeClass() throws Exception {
@Test
public void test2xBuildReload() throws Exception {
Review Comment:
I wondered about this as well. I was getting errros when run with higher multipleris...
I am going to back out the changes, and try this test again.
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2672805666
##########
solr/core/src/test/org/apache/solr/schema/SchemaVersionSpecificBehaviorTest.java:
##########
@@ -306,7 +306,6 @@ public IndexSchema initCoreUsingSchemaVersion(final float ver) throws Exception
"Schema version not set correctly", String.valueOf(ver), String.valueOf(s.getVersion()));
return s;
} finally {
- System.clearProperty("solr.schema.test.ver");
Review Comment:
thanks!
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4010:
URL: https://github.com/apache/solr/pull/4010#issuecomment-3724397218
@dsmiley I tried to "beast" the InfixSuggestersTest after reverting my changes, and it appeared to all pass. I've responded to your other feedback plus reviewed (and mostly dismissed!) the Copilot feedback.
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4010:
URL: https://github.com/apache/solr/pull/4010#issuecomment-3732663892
Thanks @dsmiley for the phone call last night. I've backed out the test improvements as not related. I've re-run the tests and I think this is ready to merge.
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4010:
URL: https://github.com/apache/solr/pull/4010#issuecomment-3724477333
> @dsmiley I tried to "beast" the InfixSuggestersTest after reverting my changes, and it appeared to all pass. I've responded to your other feedback plus reviewed (and mostly dismissed!) the Copilot feedback.
I also got some AI help and found some more empty methods that could be removed.
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2672785064
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricsIntegrationTest.java:
##########
@@ -100,7 +100,6 @@ public void testZkMetrics() throws Exception {
new MiniSolrCloudCluster.Builder(3, createTempDir())
.addConfig("conf", configset("conf2"))
.configure();
- System.clearProperty("metricsEnabled");
Review Comment:
it's odd that the test passes AFTER I remoeved that line.... I restroed the line however.
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2672762693
##########
solr/core/src/test/org/apache/solr/handler/component/DistributedCombinedQueryComponentTest.java:
##########
@@ -53,6 +53,13 @@ public static void setUpClass() {
System.setProperty("distribUpdateSoTimeout", "5000");
}
+ @Override
+ public void distribSetUp() throws Exception {
+ super.distribSetUp();
+ // Enable fuzzy comparison to handle floating-point precision differences in distributed queries
+ flags = FUZZY;
Review Comment:
Yeah, I kind of struggle with this. The test was failing, so then I asked for help and I got this.. I am a bit challenged in how I can run the full test suite with nightly tags and test multipliers and not get sucked into a million things like this...
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2672751303
##########
solr/core/src/test/org/apache/solr/cloud/TlogReplayBufferedWhileIndexingTest.java:
##########
@@ -37,7 +36,7 @@ public class TlogReplayBufferedWhileIndexingTest extends AbstractFullDistribZkTe
private List<StoppableIndexingThread> threads;
- public TlogReplayBufferedWhileIndexingTest() throws Exception {
+ public TlogReplayBufferedWhileIndexingTest() {
Review Comment:
Yeah... I find it so hard to not do this, it's almost like a nervous tic!
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2672737704
##########
solr/core/src/test/org/apache/solr/cloud/LeaderFailureAfterFreshStartTest.java:
##########
@@ -93,78 +90,71 @@ public void test() throws Exception {
handle.clear();
handle.put("timestamp", SKIPVAL);
- try {
- CloudJettyRunner initialLeaderJetty = shardToLeaderJetty.get("shard1");
- List<CloudJettyRunner> otherJetties = getOtherAvailableJetties(initialLeaderJetty);
+ CloudJettyRunner initialLeaderJetty = shardToLeaderJetty.get("shard1");
+ List<CloudJettyRunner> otherJetties = getOtherAvailableJetties(initialLeaderJetty);
+ log.info(
+ "Leader node_name: {}, url: {}", initialLeaderJetty.coreNodeName, initialLeaderJetty.url);
+ for (CloudJettyRunner cloudJettyRunner : otherJetties) {
log.info(
- "Leader node_name: {}, url: {}",
- initialLeaderJetty.coreNodeName,
- initialLeaderJetty.url);
- for (CloudJettyRunner cloudJettyRunner : otherJetties) {
- log.info(
- "Non-leader node_name: {}, url: {}",
- cloudJettyRunner.coreNodeName,
- cloudJettyRunner.url);
- }
+ "Non-leader node_name: {}, url: {}",
+ cloudJettyRunner.coreNodeName,
+ cloudJettyRunner.url);
+ }
- CloudJettyRunner secondNode = otherJetties.get(0);
- CloudJettyRunner freshNode = otherJetties.get(1);
+ CloudJettyRunner secondNode = otherJetties.get(0);
+ CloudJettyRunner freshNode = otherJetties.get(1);
- // shutdown a node to simulate fresh start
- otherJetties.remove(freshNode);
- forceNodeFailures(singletonList(freshNode));
+ // shutdown a node to simulate fresh start
+ otherJetties.remove(freshNode);
+ forceNodeFailures(singletonList(freshNode));
- del("*:*");
- waitForThingsToLevelOut(30, TimeUnit.SECONDS);
+ del("*:*");
+ waitForThingsToLevelOut(30, TimeUnit.SECONDS);
- checkShardConsistency(false, true);
+ checkShardConsistency(false, true);
- // index a few docs and commit
- for (int i = 0; i < 100; i++) {
- indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
- }
- commit();
- waitForThingsToLevelOut(30, TimeUnit.SECONDS);
-
- checkShardConsistency(false, true);
-
- // bring down the other node and index a few docs; so the leader and other node segments
- // diverge
- forceNodeFailures(singletonList(secondNode));
- for (int i = 0; i < 10; i++) {
- indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
- if (i % 2 == 0) {
- commit();
- }
+ // index a few docs and commit
+ for (int i = 0; i < 100; i++) {
+ indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
+ }
+ commit();
+ waitForThingsToLevelOut(30, TimeUnit.SECONDS);
+
+ checkShardConsistency(false, true);
+
+ // bring down the other node and index a few docs; so the leader and other node segments
+ // diverge
+ forceNodeFailures(singletonList(secondNode));
+ for (int i = 0; i < 10; i++) {
+ indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
+ if (i % 2 == 0) {
+ commit();
}
- commit();
- restartNodes(singletonList(secondNode));
-
- // start the freshNode
- restartNodes(singletonList(freshNode));
- String coreName = freshNode.jetty.getCoreContainer().getCores().iterator().next().getName();
- Path replicationProperties =
- Path.of(
- freshNode.jetty.getSolrHome(), "cores", coreName, "data", "replication.properties");
- String md5 = DigestUtils.md5Hex(Files.readAllBytes(replicationProperties));
-
- // shutdown the original leader
- log.info("Now shutting down initial leader");
- forceNodeFailures(singletonList(initialLeaderJetty));
- waitForNewLeader(cloudClient, "shard1", initialLeaderJetty.info);
- waitTillNodesActive();
- log.info("Updating mappings from zk");
- updateMappingsFromZk(jettys, clients, true);
- assertEquals(
- "Node went into replication",
- md5,
- DigestUtils.md5Hex(Files.readAllBytes(replicationProperties)));
-
- success = true;
- } finally {
- System.clearProperty("solr.index.replication.fingerprint.enabled");
}
+ commit();
+ restartNodes(singletonList(secondNode));
+
+ // start the freshNode
+ restartNodes(singletonList(freshNode));
+ String coreName = freshNode.jetty.getCoreContainer().getCores().iterator().next().getName();
+ Path replicationProperties =
+ Path.of(freshNode.jetty.getSolrHome(), "cores", coreName, "data", "replication.properties");
+ String md5 = DigestUtils.md5Hex(Files.readAllBytes(replicationProperties));
+
+ // shutdown the original leader
+ log.info("Now shutting down initial leader");
+ forceNodeFailures(singletonList(initialLeaderJetty));
+ waitForNewLeader(cloudClient, "shard1", initialLeaderJetty.info);
+ waitTillNodesActive();
+ log.info("Updating mappings from zk");
+ updateMappingsFromZk(jettys, clients, true);
+ assertEquals(
+ "Node went into replication",
+ md5,
+ DigestUtils.md5Hex(Files.readAllBytes(replicationProperties)));
+
+ success = true;
Review Comment:
I *believe* that even if the test fails, our test infrastructure will reset the `solr.index.replication.fingerprint.enabled` property.
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2672759475
##########
solr/core/src/test/org/apache/solr/handler/TestStressThreadBackup.java:
##########
@@ -81,9 +81,7 @@ public static void beforeClass() {
}
@AfterClass
- public static void afterClass() {
- System.clearProperty("solr.security.allow.paths");
- }
+ public static void afterClass() {}
Review Comment:
thanks! will do!
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2672779022
##########
solr/core/src/test/org/apache/solr/highlight/TestUnifiedSolrHighlighter.java:
##########
@@ -43,13 +43,7 @@ public static void beforeClass() throws Exception {
}
@AfterClass
- public static void afterClass() {
- System.clearProperty("filterCache.enabled");
- System.clearProperty("queryResultCache.enabled");
- System.clearProperty("documentCache.enabled");
- System.clearProperty("solr.tests.id.stored");
- System.clearProperty("solr.tests.id.docValues");
- }
+ public static void afterClass() {}
Review Comment:
done
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #4010:
URL: https://github.com/apache/solr/pull/4010
--
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] Remove unneeded clearing of system properties [solr]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2669816863
##########
solr/core/src/test/org/apache/solr/cli/ZkSubcommandsTest.java:
##########
@@ -568,34 +562,25 @@ public void testSetClusterProperty() throws Exception {
@Test
public void testUpdateAcls() throws Exception {
- try {
- System.setProperty(
- SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
- DigestZkACLProvider.class.getName());
- System.setProperty(
- VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME, "user");
- System.setProperty(
- VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME, "pass");
- System.setProperty(
- SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
- VMParamsAllAndReadonlyDigestZkACLProvider.class.getName());
- System.setProperty(
- VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME,
- "user");
- System.setProperty(
- VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME,
- "pass");
-
- String[] args = new String[] {"updateacls", "/", "-z", zkServer.getZkAddress()};
- assertEquals(0, CLITestHelper.runTool(args, UpdateACLTool.class));
- } finally {
- // Need to clear these before we open the next SolrZkClient
- System.clearProperty(SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME);
- System.clearProperty(
- VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME);
- System.clearProperty(
- VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME);
- }
+
+ System.setProperty(
+ SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME, DigestZkACLProvider.class.getName());
+ System.setProperty(
+ VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME, "user");
+ System.setProperty(
+ VMParamsZkCredentialsInjector.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME, "pass");
+ System.setProperty(
+ SolrZkClient.ZK_ACL_PROVIDER_CLASS_NAME_VM_PARAM_NAME,
+ VMParamsAllAndReadonlyDigestZkACLProvider.class.getName());
+ System.setProperty(
+ VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_USERNAME_VM_PARAM_NAME,
+ "user");
+ System.setProperty(
+ VMParamsAllAndReadonlyDigestZkACLProvider.DEFAULT_DIGEST_READONLY_PASSWORD_VM_PARAM_NAME,
+ "pass");
+
+ String[] args = new String[] {"updateacls", "/", "-z", zkServer.getZkAddress()};
+ assertEquals(0, CLITestHelper.runTool(args, UpdateACLTool.class));
Review Comment:
The removal of the try-finally block removes the protection against property leakage when updateacls tool execution fails. If the assertEquals assertion fails, the system properties will remain set, potentially affecting subsequent tests. The finally block was ensuring cleanup even when test assertions fail.
##########
solr/core/src/test/org/apache/solr/util/TestCircuitBreakers.java:
##########
@@ -165,40 +165,35 @@ public void testGlobalCBsCanBeParsedFromSystemProperties() {
props.setProperty("solr.circuitbreaker.update.cpu.warnonly", "true");
System.setProperties(props);
- try {
- final var parsedBreakers =
- CircuitBreakerRegistry.parseCircuitBreakersFromProperties(h.getCoreContainer()).stream()
- .sorted(Comparator.comparing(breaker -> breaker.toString()))
- .collect(Collectors.toList());
-
- assertEquals(3, parsedBreakers.size());
-
- assertTrue(
- "Expected CPUCircuitBreaker, but got " + parsedBreakers.get(0).getClass().getName(),
- parsedBreakers.get(0) instanceof CPUCircuitBreaker);
- final var cpuBreaker = (CPUCircuitBreaker) parsedBreakers.get(0);
- assertEquals(56.0, cpuBreaker.getCpuUsageThreshold(), 0.1);
- assertEquals(true, cpuBreaker.isWarnOnly());
- assertEquals(Set.of(SolrRequest.SolrRequestType.UPDATE), cpuBreaker.getRequestTypes());
-
- assertTrue(
- "Expected LoadAverageCircuitBreaker, but got "
- + parsedBreakers.get(1).getClass().getName(),
- parsedBreakers.get(1) instanceof LoadAverageCircuitBreaker);
- final var loadAvgBreaker = (LoadAverageCircuitBreaker) parsedBreakers.get(1);
- assertEquals(3.4, loadAvgBreaker.getLoadAverageThreshold(), 0.1);
- assertEquals(false, loadAvgBreaker.isWarnOnly());
- assertEquals(Set.of(SolrRequest.SolrRequestType.UPDATE), loadAvgBreaker.getRequestTypes());
-
- assertTrue(
- "Expected MemoryCircuitBreaker, but got " + parsedBreakers.get(2).getClass().getName(),
- parsedBreakers.get(2) instanceof MemoryCircuitBreaker);
- final var memBreaker = (MemoryCircuitBreaker) parsedBreakers.get(2);
- assertEquals(false, memBreaker.isWarnOnly());
- assertEquals(Set.of(SolrRequest.SolrRequestType.QUERY), memBreaker.getRequestTypes());
- } finally {
- props.keySet().stream().forEach(k -> System.clearProperty((String) k));
- }
+ final var parsedBreakers =
+ CircuitBreakerRegistry.parseCircuitBreakersFromProperties(h.getCoreContainer()).stream()
+ .sorted(Comparator.comparing(breaker -> breaker.toString()))
+ .collect(Collectors.toList());
+
+ assertEquals(3, parsedBreakers.size());
+
+ assertTrue(
+ "Expected CPUCircuitBreaker, but got " + parsedBreakers.get(0).getClass().getName(),
+ parsedBreakers.get(0) instanceof CPUCircuitBreaker);
+ final var cpuBreaker = (CPUCircuitBreaker) parsedBreakers.get(0);
+ assertEquals(56.0, cpuBreaker.getCpuUsageThreshold(), 0.1);
+ assertEquals(true, cpuBreaker.isWarnOnly());
+ assertEquals(Set.of(SolrRequest.SolrRequestType.UPDATE), cpuBreaker.getRequestTypes());
+
+ assertTrue(
+ "Expected LoadAverageCircuitBreaker, but got " + parsedBreakers.get(1).getClass().getName(),
+ parsedBreakers.get(1) instanceof LoadAverageCircuitBreaker);
+ final var loadAvgBreaker = (LoadAverageCircuitBreaker) parsedBreakers.get(1);
+ assertEquals(3.4, loadAvgBreaker.getLoadAverageThreshold(), 0.1);
+ assertEquals(false, loadAvgBreaker.isWarnOnly());
+ assertEquals(Set.of(SolrRequest.SolrRequestType.UPDATE), loadAvgBreaker.getRequestTypes());
+
+ assertTrue(
+ "Expected MemoryCircuitBreaker, but got " + parsedBreakers.get(2).getClass().getName(),
+ parsedBreakers.get(2) instanceof MemoryCircuitBreaker);
+ final var memBreaker = (MemoryCircuitBreaker) parsedBreakers.get(2);
+ assertEquals(false, memBreaker.isWarnOnly());
+ assertEquals(Set.of(SolrRequest.SolrRequestType.QUERY), memBreaker.getRequestTypes());
}
Review Comment:
The removal of the try-finally block changes exception handling behavior. The finally block ensures that system properties are cleared even if the test assertions fail. Without this structure, properties set within the test may leak to other tests if an exception occurs. This could cause test isolation issues and flaky test behavior.
##########
solr/core/src/test/org/apache/solr/core/TestBadConfig.java:
##########
@@ -27,13 +27,8 @@ public void testNRTModeProperty() throws Exception {
}
public void testUpdateLogButNoVersionField() throws Exception {
-
System.setProperty("solr.index.updatelog.enabled", "true");
- try {
- assertConfigs("solrconfig.xml", "schema12.xml", "_version_");
- } finally {
- System.clearProperty("solr.index.updatelog.enabled");
- }
+ assertConfigs("solrconfig.xml", "schema12.xml", "_version_");
}
Review Comment:
The removal of the try-finally block changes exception handling behavior. If an assertion fails before the end of the test, the "solr.index.updatelog.enabled" property will remain set, potentially affecting subsequent tests. The finally block was ensuring cleanup even in the case of test failures.
##########
solr/core/src/test/org/apache/solr/cloud/TlogReplayBufferedWhileIndexingTest.java:
##########
@@ -37,7 +36,7 @@ public class TlogReplayBufferedWhileIndexingTest extends AbstractFullDistribZkTe
private List<StoppableIndexingThread> threads;
- public TlogReplayBufferedWhileIndexingTest() throws Exception {
+ public TlogReplayBufferedWhileIndexingTest() {
Review Comment:
The removal of the 'throws Exception' declaration from the constructor signature changes its contract. While this may be correct if the constructor no longer throws checked exceptions, it's unrelated to the PR's stated purpose of removing clearProperty calls and should be verified as an intentional change.
##########
solr/core/src/test/org/apache/solr/handler/component/InfixSuggestersTest.java:
##########
@@ -110,9 +114,6 @@ public void testReloadDuringBuild() throws Exception {
"//str[@name='command'][.='buildAll']")));
h.reload();
// Stop the dictionary's input iterator
- System.clearProperty(
- RandomTestDictionaryFactory.RandomTestDictionary.getEnabledSysProp(
- "longRandomAnalyzingInfixSuggester"));
assertNotNull("Should have thrown exception", job.get());
Review Comment:
This System.clearProperty call is being removed from within the test loop, not from a teardown method. This appears intentional based on the added comment explaining it prevents "already in use" errors. However, the removal of the clearProperty call at lines 117-120 (after the build completes) could cause issues if the test runs multiple times or if other tests depend on this property being cleared. The property should still be cleared after each build iteration to prevent conflicts.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/MergeIndexesExampleTestBase.java:
##########
@@ -42,8 +42,6 @@
public abstract class MergeIndexesExampleTestBase extends SolrTestCaseJ4 {
protected CoreContainer cores;
- private String saveProp;
- private Path dataDir1;
private Path dataDir2;
Review Comment:
The variable 'dataDir1' is being removed from the field declarations but is still used locally in the setUp() method (line 63). This appears to be an appropriate refactoring to reduce the scope of the variable, but it's unrelated to the PR's stated purpose of removing clearProperty calls.
##########
solr/core/src/test/org/apache/solr/core/DirectoryFactoryTest.java:
##########
@@ -48,7 +47,6 @@ public static void cleanupLoader() throws Exception {
loader = null;
}
- @After
@Before
public void clean() {
System.clearProperty("solr.data.home");
Review Comment:
The removal of the @Before annotation from the 'clean()' method is significant. This method sets up test state by clearing the 'solr.data.home' property before each test. By removing @Before and keeping @After, the cleanup now happens after tests instead of before. This could cause test failures if tests depend on this property being cleared beforehand. This appears to be a mistake in the refactoring.
##########
solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java:
##########
@@ -59,6 +59,7 @@ public void setupCluster() throws Exception {
System.setProperty("solr.directoryFactory", "solr.StandardDirectoryFactory");
System.setProperty("solr.ulog.numRecordsToKeep", "1000");
System.setProperty("leaderVoteWait", "60000");
+ System.setProperty("tests.zk.violationReportAction", "IGNORE");
Review Comment:
A new system property is being set here that wasn't present in the original code. This appears to be an intentional addition to handle ZooKeeper violation reporting, but it's unrelated to the PR's stated purpose of removing clearProperty calls from teardown methods. This change should be documented or moved to a separate PR.
```suggestion
if (System.getProperty("tests.zk.violationReportAction") == null) {
System.setProperty("tests.zk.violationReportAction", "IGNORE");
}
```
##########
solr/core/src/test/org/apache/solr/handler/component/InfixSuggestersTest.java:
##########
@@ -151,9 +152,6 @@ public void testShutdownDuringBuild() throws Exception {
Thread.sleep(100); // TODO: is there a better way to ensure that the build has begun?
h.close();
// Stop the dictionary's input iterator
- System.clearProperty(
- RandomTestDictionaryFactory.RandomTestDictionary.getEnabledSysProp(
- "longRandomAnalyzingInfixSuggester"));
job.get();
Review Comment:
This System.clearProperty call is being removed from within the test loop, not from a teardown method. The removal appears to be a mistake - this property should be cleared after the dictionary's input iterator is stopped to clean up the test state properly. This is different from the other changes in the PR which remove clearProperty calls from teardown methods.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/MergeIndexesExampleTestBase.java:
##########
@@ -60,7 +58,6 @@ protected void setupCoreContainer() {
@Override
public void setUp() throws Exception {
- saveProp = System.getProperty("solr.directoryFactory");
System.setProperty("solr.directoryFactory", "solr.StandardDirectoryFactory");
Review Comment:
The variable 'saveProp' is being removed but was used to restore the original 'solr.directoryFactory' property value. With the removal of the finally block that restored this property, the original behavior of preserving and restoring the property is lost. While the test framework may handle this, removing the restoration logic could affect test isolation if the framework doesn't clear this specific property.
##########
solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java:
##########
@@ -250,10 +238,10 @@ public void testMergedSegmentWarmerIndexConfigCreation() throws Exception {
}
public void testToMap() throws Exception {
- final String solrConfigFileNameWarmer = solrConfigFileNameWarmerRandomMergePolicyFactory;
- final String solrConfigFileNameTMP = solrConfigFileNameTieredMergePolicyFactory;
final String solrConfigFileName =
- (random().nextBoolean() ? solrConfigFileNameWarmer : solrConfigFileNameTMP);
+ (random().nextBoolean()
+ ? solrConfigFileNameWarmerRandomMergePolicyFactory
+ : solrConfigFileNameTieredMergePolicyFactory);
Review Comment:
This is an inline variable refactoring where temporary variables are removed in favor of using the values directly in a ternary expression. While this is a minor code improvement, it's unrelated to the PR's stated purpose of removing clearProperty calls and introduces unnecessary scope creep.
##########
solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java:
##########
@@ -177,7 +166,6 @@ public void testSortingMPSolrIndexConfigCreation() throws Exception {
}
public void testMergeOnFlushMPSolrIndexConfigCreation() throws Exception {
- final SortField sortField1 = new SortField("timestamp_i_dvo", SortField.Type.INT, true);
final SortField sortField2 = new SortField("id", SortField.Type.STRING, false);
Review Comment:
The variable 'sortField1' is removed but was never used in the original code. This cleanup is appropriate but unrelated to the PR's stated purpose of removing clearProperty calls.
##########
solr/core/src/test/org/apache/solr/handler/component/DistributedCombinedQueryComponentTest.java:
##########
@@ -53,6 +53,13 @@ public static void setUpClass() {
System.setProperty("distribUpdateSoTimeout", "5000");
}
+ @Override
+ public void distribSetUp() throws Exception {
+ super.distribSetUp();
+ // Enable fuzzy comparison to handle floating-point precision differences in distributed queries
+ flags = FUZZY;
+ }
Review Comment:
A new method 'distribSetUp' is being added that sets the FUZZY flag. This appears to be a functional change that's unrelated to the PR's stated purpose of removing clearProperty calls from teardown methods. This change should be documented or moved to a separate PR as it modifies test behavior.
##########
solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java:
##########
@@ -127,8 +117,7 @@ public void testTieredMPSolrIndexConfigCreation() throws Exception {
@Test
public void testConcurrentMergeSchedularSolrIndexConfigCreation() throws Exception {
- String solrConfigFileName = solrConfigFileNameConnMSPolicyFactory;
- SolrConfig solrConfig = new SolrConfig(instanceDir, solrConfigFileName);
+ SolrConfig solrConfig = new SolrConfig(instanceDir, solrConfigFileNameConnMSPolicyFactory);
Review Comment:
This is an inline variable refactoring where a temporary variable is removed in favor of using the value directly. While this is a minor code improvement, it's unrelated to the PR's stated purpose of removing clearProperty calls and introduces unnecessary scope creep.
##########
solr/core/src/test/org/apache/solr/core/TestTracerConfigurator.java:
##########
@@ -19,24 +19,17 @@
import io.opentelemetry.api.trace.TracerProvider;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.util.tracing.TraceUtils;
-import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
public class TestTracerConfigurator extends SolrTestCaseJ4 {
@BeforeClass
- public static void setUpProperties() throws Exception {
+ public static void setUpProperties() {
Review Comment:
The removal of the 'throws Exception' declaration from the method signature changes the method's contract. While this may be correct if the method no longer throws checked exceptions, it's unrelated to the PR's stated purpose of removing clearProperty calls and should be verified as an intentional change.
##########
solr/core/src/test/org/apache/solr/cloud/PeerSyncReplicationTest.java:
##########
@@ -114,107 +110,101 @@ public void test() throws Exception {
commit();
waitForThingsToLevelOut(30, TimeUnit.SECONDS);
- try {
- checkShardConsistency(false, true);
+ checkShardConsistency(false, true);
- long cloudClientDocs = cloudClient.query(new SolrQuery("*:*")).getResults().getNumFound();
- assertEquals(docId, cloudClientDocs);
+ long cloudClientDocs = cloudClient.query(new SolrQuery("*:*")).getResults().getNumFound();
+ assertEquals(docId, cloudClientDocs);
- CloudJettyRunner initialLeaderJetty = shardToLeaderJetty.get("shard1");
- List<CloudJettyRunner> otherJetties = getOtherAvailableJetties(initialLeaderJetty);
- CloudJettyRunner neverLeader = otherJetties.get(otherJetties.size() - 1);
- otherJetties.remove(neverLeader);
+ CloudJettyRunner initialLeaderJetty = shardToLeaderJetty.get("shard1");
+ List<CloudJettyRunner> otherJetties = getOtherAvailableJetties(initialLeaderJetty);
+ CloudJettyRunner neverLeader = otherJetties.get(otherJetties.size() - 1);
+ otherJetties.remove(neverLeader);
- // first shutdown a node that will never be a leader
- forceNodeFailures(singletonList(neverLeader));
+ // first shutdown a node that will never be a leader
+ forceNodeFailures(singletonList(neverLeader));
- // node failure and recovery via PeerSync
- log.info("Forcing PeerSync");
- CloudJettyRunner nodePeerSynced = forceNodeFailureAndDoPeerSync(true);
+ // node failure and recovery via PeerSync
+ log.info("Forcing PeerSync");
+ CloudJettyRunner nodePeerSynced = forceNodeFailureAndDoPeerSync(true);
- // add a few more docs
- indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
- indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
- commit();
-
- cloudClientDocs = cloudClient.query(new SolrQuery("*:*")).getResults().getNumFound();
- assertEquals(docId, cloudClientDocs);
-
- // now shutdown all other nodes except for 'nodeShutDownForFailure'
- otherJetties.remove(nodePeerSynced);
- forceNodeFailures(otherJetties);
- waitForThingsToLevelOut(30, TimeUnit.SECONDS);
- checkShardConsistency(false, true);
-
- // now shutdown the original leader
- log.info("Now shutting down initial leader");
- forceNodeFailures(singletonList(initialLeaderJetty));
- log.info("Updating mappings from zk");
- waitForNewLeader(cloudClient, "shard1", initialLeaderJetty.info);
- updateMappingsFromZk(jettys, clients, true);
- assertEquals(
- "PeerSynced node did not become leader",
- nodePeerSynced,
- shardToLeaderJetty.get("shard1"));
-
- // bring up node that was down all along, and let it PeerSync from the node that was forced to
- // PeerSync
- bringUpDeadNodeAndEnsureNoReplication(neverLeader, false);
- waitTillNodesActive();
-
- checkShardConsistency(false, true);
-
- // bring back all the nodes including initial leader
- // (commented as reports Maximum concurrent create/delete watches above limit violation and
- // reports thread leaks)
- /*for(int i = 0 ; i < nodesDown.size(); i++) {
- bringUpDeadNodeAndEnsureNoReplication(shardToLeaderJetty.get("shard1"), neverLeader, false);
- }
- checkShardConsistency(false, true);*/
+ // add a few more docs
+ indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
+ indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
+ commit();
+
+ cloudClientDocs = cloudClient.query(new SolrQuery("*:*")).getResults().getNumFound();
+ assertEquals(docId, cloudClientDocs);
- // make sure leader has not changed after bringing initial leader back
- assertEquals(nodePeerSynced, shardToLeaderJetty.get("shard1"));
+ // now shutdown all other nodes except for 'nodeShutDownForFailure'
+ otherJetties.remove(nodePeerSynced);
+ forceNodeFailures(otherJetties);
+ waitForThingsToLevelOut(30, TimeUnit.SECONDS);
+ checkShardConsistency(false, true);
+
+ // now shutdown the original leader
+ log.info("Now shutting down initial leader");
+ forceNodeFailures(singletonList(initialLeaderJetty));
+ log.info("Updating mappings from zk");
+ waitForNewLeader(cloudClient, "shard1", initialLeaderJetty.info);
+ updateMappingsFromZk(jettys, clients, true);
+ assertEquals(
+ "PeerSynced node did not become leader", nodePeerSynced, shardToLeaderJetty.get("shard1"));
+
+ // bring up node that was down all along, and let it PeerSync from the node that was forced to
+ // PeerSync
+ bringUpDeadNodeAndEnsureNoReplication(neverLeader, false);
+ waitTillNodesActive();
- // assert metrics
- SolrMetricManager manager = nodePeerSynced.jetty.getCoreContainer().getMetricManager();
- MeterProvider registry = null;
- for (String name : manager.registryNames()) {
- if (name.startsWith("solr.core.collection1")) {
- registry = manager.meterProvider(name);
- break;
- }
- }
- assertNotNull(registry);
- CoreContainer cc = nodePeerSynced.jetty.getCoreContainer();
- String coreName =
- cc.getAllCoreNames().stream()
- .filter(n -> n.contains(DEFAULT_TEST_COLLECTION_NAME))
- .findFirst()
- .orElseThrow(
- () ->
- new IllegalStateException(
- "Couldn't find core for " + nodePeerSynced.coreNodeName));
- try (SolrCore core = cc.getCore(coreName)) {
- assertTrue(
- SolrMetricTestUtils.getHistogramDatapoint(
- core,
- "solr_core_sync_with_leader_time_milliseconds",
- SolrMetricTestUtils.newCloudLabelsBuilder(core)
- .label("category", "REPLICATION")
- .build())
- .hasCount());
- assertNull(
- SolrMetricTestUtils.getCounterDatapoint(
- core,
- "solr_core_sync_with_leader_sync_errors",
- SolrMetricTestUtils.newCloudLabelsBuilder(core)
- .label("category", "REPLICATION")
- .build()));
+ checkShardConsistency(false, true);
+
+ // bring back all the nodes including initial leader
+ // (commented as reports Maximum concurrent create/delete watches above limit violation and
+ // reports thread leaks)
+ /*for(int i = 0 ; i < nodesDown.size(); i++) {
+ bringUpDeadNodeAndEnsureNoReplication(shardToLeaderJetty.get("shard1"), neverLeader, false);
+ }
+ checkShardConsistency(false, true);*/
+
+ // make sure leader has not changed after bringing initial leader back
+ assertEquals(nodePeerSynced, shardToLeaderJetty.get("shard1"));
+
+ // assert metrics
+ SolrMetricManager manager = nodePeerSynced.jetty.getCoreContainer().getMetricManager();
+ MeterProvider registry = null;
+ for (String name : manager.registryNames()) {
+ if (name.startsWith("solr.core.collection1")) {
+ registry = manager.meterProvider(name);
+ break;
}
- success = true;
- } finally {
- System.clearProperty("solr.index.replication.fingerprint.enabled");
}
+ assertNotNull(registry);
+ CoreContainer cc = nodePeerSynced.jetty.getCoreContainer();
+ String coreName =
+ cc.getAllCoreNames().stream()
+ .filter(n -> n.contains(DEFAULT_TEST_COLLECTION_NAME))
+ .findFirst()
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ "Couldn't find core for " + nodePeerSynced.coreNodeName));
+ try (SolrCore core = cc.getCore(coreName)) {
+ assertTrue(
+ SolrMetricTestUtils.getHistogramDatapoint(
+ core,
+ "solr_core_sync_with_leader_time_milliseconds",
+ SolrMetricTestUtils.newCloudLabelsBuilder(core)
+ .label("category", "REPLICATION")
+ .build())
+ .hasCount());
+ assertNull(
+ SolrMetricTestUtils.getCounterDatapoint(
+ core,
+ "solr_core_sync_with_leader_sync_errors",
+ SolrMetricTestUtils.newCloudLabelsBuilder(core)
+ .label("category", "REPLICATION")
+ .build()));
+ }
+ success = true;
Review Comment:
The removal of the try-finally block that previously wrapped the test logic changes exception handling behavior. If test assertions fail, the "solr.index.replication.fingerprint.enabled" property will remain set in system properties, potentially affecting subsequent tests. The success flag and finally block structure were ensuring proper cleanup even in case of test failures.
```suggestion
try {
try (SolrCore core = cc.getCore(coreName)) {
assertTrue(
SolrMetricTestUtils.getHistogramDatapoint(
core,
"solr_core_sync_with_leader_time_milliseconds",
SolrMetricTestUtils.newCloudLabelsBuilder(core)
.label("category", "REPLICATION")
.build())
.hasCount());
assertNull(
SolrMetricTestUtils.getCounterDatapoint(
core,
"solr_core_sync_with_leader_sync_errors",
SolrMetricTestUtils.newCloudLabelsBuilder(core)
.label("category", "REPLICATION")
.build()));
}
success = true;
} finally {
System.clearProperty("solr.index.replication.fingerprint.enabled");
}
```
##########
solr/core/src/test/org/apache/solr/cloud/LeaderFailureAfterFreshStartTest.java:
##########
@@ -93,78 +90,71 @@ public void test() throws Exception {
handle.clear();
handle.put("timestamp", SKIPVAL);
- try {
- CloudJettyRunner initialLeaderJetty = shardToLeaderJetty.get("shard1");
- List<CloudJettyRunner> otherJetties = getOtherAvailableJetties(initialLeaderJetty);
+ CloudJettyRunner initialLeaderJetty = shardToLeaderJetty.get("shard1");
+ List<CloudJettyRunner> otherJetties = getOtherAvailableJetties(initialLeaderJetty);
+ log.info(
+ "Leader node_name: {}, url: {}", initialLeaderJetty.coreNodeName, initialLeaderJetty.url);
+ for (CloudJettyRunner cloudJettyRunner : otherJetties) {
log.info(
- "Leader node_name: {}, url: {}",
- initialLeaderJetty.coreNodeName,
- initialLeaderJetty.url);
- for (CloudJettyRunner cloudJettyRunner : otherJetties) {
- log.info(
- "Non-leader node_name: {}, url: {}",
- cloudJettyRunner.coreNodeName,
- cloudJettyRunner.url);
- }
+ "Non-leader node_name: {}, url: {}",
+ cloudJettyRunner.coreNodeName,
+ cloudJettyRunner.url);
+ }
- CloudJettyRunner secondNode = otherJetties.get(0);
- CloudJettyRunner freshNode = otherJetties.get(1);
+ CloudJettyRunner secondNode = otherJetties.get(0);
+ CloudJettyRunner freshNode = otherJetties.get(1);
- // shutdown a node to simulate fresh start
- otherJetties.remove(freshNode);
- forceNodeFailures(singletonList(freshNode));
+ // shutdown a node to simulate fresh start
+ otherJetties.remove(freshNode);
+ forceNodeFailures(singletonList(freshNode));
- del("*:*");
- waitForThingsToLevelOut(30, TimeUnit.SECONDS);
+ del("*:*");
+ waitForThingsToLevelOut(30, TimeUnit.SECONDS);
- checkShardConsistency(false, true);
+ checkShardConsistency(false, true);
- // index a few docs and commit
- for (int i = 0; i < 100; i++) {
- indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
- }
- commit();
- waitForThingsToLevelOut(30, TimeUnit.SECONDS);
-
- checkShardConsistency(false, true);
-
- // bring down the other node and index a few docs; so the leader and other node segments
- // diverge
- forceNodeFailures(singletonList(secondNode));
- for (int i = 0; i < 10; i++) {
- indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
- if (i % 2 == 0) {
- commit();
- }
+ // index a few docs and commit
+ for (int i = 0; i < 100; i++) {
+ indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
+ }
+ commit();
+ waitForThingsToLevelOut(30, TimeUnit.SECONDS);
+
+ checkShardConsistency(false, true);
+
+ // bring down the other node and index a few docs; so the leader and other node segments
+ // diverge
+ forceNodeFailures(singletonList(secondNode));
+ for (int i = 0; i < 10; i++) {
+ indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
+ if (i % 2 == 0) {
+ commit();
}
- commit();
- restartNodes(singletonList(secondNode));
-
- // start the freshNode
- restartNodes(singletonList(freshNode));
- String coreName = freshNode.jetty.getCoreContainer().getCores().iterator().next().getName();
- Path replicationProperties =
- Path.of(
- freshNode.jetty.getSolrHome(), "cores", coreName, "data", "replication.properties");
- String md5 = DigestUtils.md5Hex(Files.readAllBytes(replicationProperties));
-
- // shutdown the original leader
- log.info("Now shutting down initial leader");
- forceNodeFailures(singletonList(initialLeaderJetty));
- waitForNewLeader(cloudClient, "shard1", initialLeaderJetty.info);
- waitTillNodesActive();
- log.info("Updating mappings from zk");
- updateMappingsFromZk(jettys, clients, true);
- assertEquals(
- "Node went into replication",
- md5,
- DigestUtils.md5Hex(Files.readAllBytes(replicationProperties)));
-
- success = true;
- } finally {
- System.clearProperty("solr.index.replication.fingerprint.enabled");
}
+ commit();
+ restartNodes(singletonList(secondNode));
+
+ // start the freshNode
+ restartNodes(singletonList(freshNode));
+ String coreName = freshNode.jetty.getCoreContainer().getCores().iterator().next().getName();
+ Path replicationProperties =
+ Path.of(freshNode.jetty.getSolrHome(), "cores", coreName, "data", "replication.properties");
+ String md5 = DigestUtils.md5Hex(Files.readAllBytes(replicationProperties));
+
+ // shutdown the original leader
+ log.info("Now shutting down initial leader");
+ forceNodeFailures(singletonList(initialLeaderJetty));
+ waitForNewLeader(cloudClient, "shard1", initialLeaderJetty.info);
+ waitTillNodesActive();
+ log.info("Updating mappings from zk");
+ updateMappingsFromZk(jettys, clients, true);
+ assertEquals(
+ "Node went into replication",
+ md5,
+ DigestUtils.md5Hex(Files.readAllBytes(replicationProperties)));
+
+ success = true;
Review Comment:
The removal of the try-finally block changes exception handling behavior. If test assertions fail, the "solr.index.replication.fingerprint.enabled" property will remain set, potentially affecting subsequent tests. The success flag tracking and finally block were ensuring proper cleanup even when the test fails.
```suggestion
try {
forceNodeFailures(singletonList(initialLeaderJetty));
waitForNewLeader(cloudClient, "shard1", initialLeaderJetty.info);
waitTillNodesActive();
log.info("Updating mappings from zk");
updateMappingsFromZk(jettys, clients, true);
assertEquals(
"Node went into replication",
md5,
DigestUtils.md5Hex(Files.readAllBytes(replicationProperties)));
success = true;
} finally {
// Ensure this test does not leave the fingerprint feature enabled for subsequent tests
System.clearProperty("solr.index.replication.fingerprint.enabled");
}
```
##########
solr/core/src/test/org/apache/solr/update/SolrIndexConfigTest.java:
##########
@@ -103,8 +94,7 @@ public void testFailingSolrIndexConfigCreation() throws Exception {
@Test
public void testTieredMPSolrIndexConfigCreation() throws Exception {
- String solrConfigFileName = solrConfigFileNameTieredMergePolicyFactory;
- SolrConfig solrConfig = new SolrConfig(instanceDir, solrConfigFileName);
+ SolrConfig solrConfig = new SolrConfig(instanceDir, solrConfigFileNameTieredMergePolicyFactory);
Review Comment:
This is an inline variable refactoring where a temporary variable is removed in favor of using the value directly. While this is a minor code improvement, it's unrelated to the PR's stated purpose of removing clearProperty calls and introduces unnecessary scope creep.
##########
solr/core/src/test/org/apache/solr/util/TestCircuitBreakers.java:
##########
@@ -212,7 +207,6 @@ public void testCBAlwaysTripsInvalidErrorCodeSysProp() {
SolrException ex =
expectThrows(SolrException.class, () -> new MockCircuitBreaker(true));
assertTrue(ex.getMessage().contains("Invalid error code"));
- System.clearProperty(CircuitBreaker.SYSPROP_SOLR_CIRCUITBREAKER_ERRORCODE);
});
Review Comment:
The removal of the try-finally block changes exception handling behavior. If an exception is thrown during test execution, the SYSPROP_SOLR_CIRCUITBREAKER_ERRORCODE property will not be cleared, which could affect subsequent tests. The finally block was ensuring proper cleanup even in the case of assertion failures.
##########
solr/test-framework/src/test/org/apache/solr/cloud/MiniSolrCloudClusterTest.java:
##########
@@ -189,47 +189,44 @@ public void testMultipleClustersDiffZk() throws Exception {
}
public void testJettyUsingSysProp() throws Exception {
- try {
- // this cluster will use a system property to communicate zkHost to its nodes -- not node
- // props in
- // the servlet context
- final MiniSolrCloudCluster x =
- new MiniSolrCloudCluster(1, createTempDir(), JettyConfig.builder().build()) {
- @Override
- public JettySolrRunner startJettySolrRunner(
- String name, JettyConfig config, String solrXml) throws Exception {
- System.setProperty("zkHost", getZkServer().getZkAddress());
-
- final Properties nodeProps = new Properties();
- nodeProps.setProperty("test-from-sysprop", "yup");
-
- Path runnerPath = createTempDir(name);
- if (solrXml == null) {
- solrXml = DEFAULT_CLOUD_SOLR_XML;
- }
- Files.write(runnerPath.resolve("solr.xml"), solrXml.getBytes(StandardCharsets.UTF_8));
- JettyConfig newConfig = JettyConfig.builder(config).build();
- JettySolrRunner jetty =
- new JettySolrRunner(runnerPath.toString(), nodeProps, newConfig);
- return super.startJettySolrRunner(jetty);
+
+ // this cluster will use a system property to communicate zkHost to its nodes -- not node
+ // props in
+ // the servlet context
+ final MiniSolrCloudCluster x =
+ new MiniSolrCloudCluster(1, createTempDir(), JettyConfig.builder().build()) {
+ @Override
+ public JettySolrRunner startJettySolrRunner(
+ String name, JettyConfig config, String solrXml) throws Exception {
+ System.setProperty("zkHost", getZkServer().getZkAddress());
+
+ final Properties nodeProps = new Properties();
+ nodeProps.setProperty("test-from-sysprop", "yup");
+
+ Path runnerPath = createTempDir(name);
+ if (solrXml == null) {
+ solrXml = DEFAULT_CLOUD_SOLR_XML;
}
- };
- try {
- // baseline check
- assertEquals(1, x.getJettySolrRunners().size());
- assertZkHost("x", x.getZkServer().getZkAddress(), x.getJettySolrRunners().get(0));
+ Files.write(runnerPath.resolve("solr.xml"), solrXml.getBytes(StandardCharsets.UTF_8));
+ JettyConfig newConfig = JettyConfig.builder(config).build();
+ JettySolrRunner jetty =
+ new JettySolrRunner(runnerPath.toString(), nodeProps, newConfig);
+ return super.startJettySolrRunner(jetty);
+ }
+ };
+ try {
+ // baseline check
+ assertEquals(1, x.getJettySolrRunners().size());
+ assertZkHost("x", x.getZkServer().getZkAddress(), x.getJettySolrRunners().get(0));
- // verify MiniSolrCloudCluster's impl didn't change out from under us making test useless
- assertEquals(
- "yup",
- x.getJettySolrRunners().get(0).getNodeProperties().getProperty("test-from-sysprop"));
- assertNull(x.getJettySolrRunners().get(0).getNodeProperties().getProperty("zkHost"));
+ // verify MiniSolrCloudCluster's impl didn't change out from under us making test useless
+ assertEquals(
+ "yup",
+ x.getJettySolrRunners().get(0).getNodeProperties().getProperty("test-from-sysprop"));
+ assertNull(x.getJettySolrRunners().get(0).getNodeProperties().getProperty("zkHost"));
- } finally {
- x.shutdown();
- }
} finally {
- System.clearProperty("zkHost");
+ x.shutdown();
}
Review Comment:
The removal of the try-finally block changes the exception handling behavior of this test. If an exception occurs during the test execution, the zkHost property will remain set in the system properties, potentially affecting subsequent tests. While the test infrastructure may clear properties after test completion, removing the try-finally block could cause issues if the test framework doesn't handle exceptions in the same way. Consider keeping the try-finally structure even if the clearProperty call is removed.
##########
solr/core/src/test/org/apache/solr/core/TestCodecSupport.java:
##########
@@ -120,12 +120,8 @@ private void doTestCompressionMode(String propertyValue, String expectedModeStri
if (propertyValue != null) {
System.setProperty("tests.COMPRESSION_MODE", propertyValue);
}
- try {
- reloadCoreAndRecreateIndex();
- assertCompressionMode(expectedModeString, h.getCore());
- } finally {
- System.clearProperty("tests.COMPRESSION_MODE");
- }
+ reloadCoreAndRecreateIndex();
+ assertCompressionMode(expectedModeString, h.getCore());
}
Review Comment:
The removal of the try-finally block changes exception handling behavior. If the test fails, the "tests.COMPRESSION_MODE" property will remain set in the system. While the test framework may clear it, removing the explicit cleanup in the finally block could cause test isolation issues if subsequent tests depend on this property being unset.
##########
solr/modules/cross-dc/src/test/org/apache/solr/crossdc/handler/MirroringCollectionsHandlerTest.java:
##########
@@ -60,7 +59,6 @@ public class MirroringCollectionsHandlerTest extends SolrTestCaseJ4 {
private final ZkController zkController = Mockito.mock(ZkController.class);
private final SolrZkClient solrZkClient = Mockito.mock(SolrZkClient.class);
- @SuppressWarnings("unchecked")
private ArgumentCaptor<MirroredSolrRequest<?>> captor;
Review Comment:
The removal of the @SuppressWarnings annotation changes how the compiler handles unchecked warnings for this field. While this may be correct if the warning is no longer applicable, it's unrelated to the PR's stated purpose of removing clearProperty calls and should be verified as intentional.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/MergeIndexesExampleTestBase.java:
##########
@@ -165,7 +159,7 @@ public void testMergeIndexesByCoreName() throws Exception {
assertEquals(1, getSolrCore0().query(new SolrQuery("id:BBB")).getResults().size());
}
- public void testMergeMultipleRequest() throws Exception {
+ public void testMergeMultipleRequest() {
Review Comment:
The removal of the 'throws Exception' declaration from the method signature changes the method's contract. While this may be correct if the method no longer throws checked exceptions, it's unrelated to the PR's stated purpose and should be verified.
##########
solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java:
##########
@@ -265,11 +265,7 @@ public void testUrlAllowList() throws Exception {
// Expect the same test to pass now.
System.setProperty(
TEST_URL_ALLOW_LIST, leaderJetty.getBaseUrl() + "," + followerJetty.getBaseUrl());
- try {
- doTestDetails();
- } finally {
- System.clearProperty(TEST_URL_ALLOW_LIST);
- }
+ doTestDetails();
}
Review Comment:
The removal of the try-finally block changes exception handling behavior. The finally block ensures that the TEST_URL_ALLOW_LIST property is cleared even if the test fails. Without it, the property may leak to subsequent tests if doTestDetails() throws an exception, potentially causing test isolation issues.
##########
solr/core/src/test/org/apache/solr/servlet/HideStackTraceTest.java:
##########
@@ -143,8 +137,7 @@ public void testHideStackTrace() throws Exception {
// }
// }
- final String url =
- solrTestRule.getBaseUrl().toString() + "/collection1/withError?q=*:*&wt=json";
+ final String url = solrTestRule.getBaseUrl() + "/collection1/withError?q=*:*&wt=json";
Review Comment:
The toString() method call is being removed from the getBaseUrl() call. This changes the return type from String to whatever getBaseUrl() returns. This appears to be a functional change unrelated to the PR's purpose. If getBaseUrl() already returns a String, this is acceptable cleanup, but if it returns a different type (like URL), this could cause compilation or runtime issues.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java:
##########
@@ -40,7 +40,6 @@
import org.apache.solr.client.solrj.RemoteSolrException;
import org.apache.solr.client.solrj.SolrRequest;
import org.apache.solr.client.solrj.SolrServerException;
-import org.apache.solr.client.solrj.jetty.HttpJettySolrClient;
import org.apache.solr.client.solrj.request.QueryRequest;
Review Comment:
The HttpJettySolrClient import is being removed. This appears to be an unused import cleanup, which is appropriate but unrelated to the PR's stated purpose of removing clearProperty calls.
##########
solr/core/src/test/org/apache/solr/core/TestTracerConfigurator.java:
##########
@@ -51,10 +44,6 @@ public void configuratorClassDoesNotExistTest() {
@Test
public void otelDisabledByProperty() {
System.setProperty("otel.sdk.disabled", "true");
- try {
- assertFalse(OpenTelemetryConfigurator.shouldAutoConfigOTEL());
- } finally {
- System.clearProperty("otel.sdk.disabled");
- }
+ assertFalse(OpenTelemetryConfigurator.shouldAutoConfigOTEL());
}
Review Comment:
The removal of the try-finally block changes exception handling behavior. If an assertion in the test method fails, the "otel.sdk.disabled" property will remain set to "true", which could affect subsequent tests that rely on OpenTelemetry. The finally block was ensuring this property is cleared even when test assertions fail.
##########
solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractCloudBackupRestoreTestCase.java:
##########
@@ -115,7 +109,6 @@ public void test() throws Exception {
replFactor = TestUtil.nextInt(random(), 1, 2);
numTlogReplicas = TestUtil.nextInt(random(), 0, 1);
numPullReplicas = TestUtil.nextInt(random(), 0, 1);
- int backupReplFactor = replFactor + numPullReplicas + numTlogReplicas;
Review Comment:
The variable 'backupReplFactor' is declared but never used after this line. This appears to be dead code that should be removed entirely rather than just having its assignment removed.
##########
solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java:
##########
@@ -274,7 +272,7 @@ private void addDocWhenOtherReplicasAreNetworkPartitioned(
proxies.get(cluster.getJettySolrRunner(0)).reopen();
cluster.getJettySolrRunner(0).start();
cluster.waitForAllNodes(30);
- ;
+
Review Comment:
This appears to be a formatting change where an extra semicolon was removed. While this is a minor improvement, it's unrelated to the PR's purpose of removing clearProperty calls and could be considered outside the scope of this change.
##########
solr/core/src/test/org/apache/solr/handler/component/ShardsAllowListTest.java:
##########
@@ -70,7 +69,7 @@ private static void appendClusterNodes(
}
@BeforeClass
- public static void setupClusters() throws Exception {
+ public static void setupClusters() {
Review Comment:
The removal of the 'throws Exception' declaration from the method signature changes the method's contract. While this may be correct if the method no longer throws checked exceptions, it's unrelated to the PR's stated purpose of removing clearProperty calls and should be verified as an intentional change.
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4010:
URL: https://github.com/apache/solr/pull/4010#issuecomment-3720401998
I have run this locally a few times with `gw test -Ptests.multiplier=4` and it's passed. I'm going to try to run it with `gw test -Ptests.multiplier=3 -Ptests.useSecurityManager=true -Ptests.nightly=true` and see what happens.
I don't want to try and fix a million flaky tests, but I do want to confirm this change is good, and that our test infrastructure is working.. Not sure quite how to do that!
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2662374872
##########
solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java:
##########
@@ -59,6 +59,7 @@ public void setupCluster() throws Exception {
System.setProperty("solr.directoryFactory", "solr.StandardDirectoryFactory");
System.setProperty("solr.ulog.numRecordsToKeep", "1000");
System.setProperty("leaderVoteWait", "60000");
+ System.setProperty("tests.zk.violationReportAction", "IGNORE");
Review Comment:
Wasn't sure where to document this, but I asked Claude for help on why the test failed sometimes, but not when I run just the single test with the seed. The response was:
The issue you encountered was due to ZooKeeper watch limits in the test framework. The `TestCloudConsistency` test creates a complex cloud setup with 4 nodes, network proxies, and performs various cluster operations that create many ZooKeeper watches. The test framework's default watch limit was set very low (1), causing the test to fail with "Maximum concurrent create/delete watches above limit" errors.
- TestCloudConsistency` was missing the system property `tests.zk.violationReportAction=IGNORE` that other similar complex cloud tests use
- The test creates many ZooKeeper watches during cluster operations (collection creation, replica management, network partitioning, etc.)
- The default watch limits in the test framework were too restrictive for this test
--
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] Remove unneeded clearing of system properties [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2670998786
##########
solr/core/src/test/org/apache/solr/handler/TestStressThreadBackup.java:
##########
@@ -81,9 +81,7 @@ public static void beforeClass() {
}
@AfterClass
- public static void afterClass() {
- System.clearProperty("solr.security.allow.paths");
- }
+ public static void afterClass() {}
Review Comment:
remove
##########
solr/core/src/test/org/apache/solr/schema/ManagedSchemaRoundRobinCloudTest.java:
##########
@@ -54,9 +54,7 @@ public static void setupCluster() throws Exception {
}
@AfterClass
- public static void clearSysProps() {
- System.clearProperty("managed.schema.mutable");
- }
+ public static void clearSysProps() {}
Review Comment:
remove
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricsIntegrationTest.java:
##########
@@ -100,7 +100,6 @@ public void testZkMetrics() throws Exception {
new MiniSolrCloudCluster.Builder(3, createTempDir())
.addConfig("conf", configset("conf2"))
.configure();
- System.clearProperty("metricsEnabled");
Review Comment:
I liked this line as it showed the limited scope of impact of when/where the property had an effect.
##########
solr/core/src/test/org/apache/solr/highlight/TestUnifiedSolrHighlighterWithoutStoredId.java:
##########
@@ -29,8 +29,5 @@ public static void beforeClassProps() {
}
@AfterClass
- public static void afterClassProps() {
- System.clearProperty("solr.tests.id.stored");
- System.clearProperty("solr.tests.id.docValues");
- }
+ public static void afterClassProps() {}
Review Comment:
remove
##########
solr/core/src/test/org/apache/solr/handler/component/InfixSuggestersTest.java:
##########
@@ -44,6 +44,10 @@ public static void beforeClass() throws Exception {
@Test
public void test2xBuildReload() throws Exception {
Review Comment:
weird difference. Can't we just leave this test alone?
##########
solr/core/src/test/org/apache/solr/handler/component/DistributedCombinedQueryComponentTest.java:
##########
@@ -53,6 +53,13 @@ public static void setUpClass() {
System.setProperty("distribUpdateSoTimeout", "5000");
}
+ @Override
+ public void distribSetUp() throws Exception {
+ super.distribSetUp();
+ // Enable fuzzy comparison to handle floating-point precision differences in distributed queries
+ flags = FUZZY;
Review Comment:
interesting addition.
##########
solr/core/src/test/org/apache/solr/schema/SchemaVersionSpecificBehaviorTest.java:
##########
@@ -306,7 +306,6 @@ public IndexSchema initCoreUsingSchemaVersion(final float ver) throws Exception
"Schema version not set correctly", String.valueOf(ver), String.valueOf(s.getVersion()));
return s;
} finally {
- System.clearProperty("solr.schema.test.ver");
Review Comment:
left an empty finally block
##########
solr/core/src/test/org/apache/solr/highlight/TestUnifiedSolrHighlighter.java:
##########
@@ -43,13 +43,7 @@ public static void beforeClass() throws Exception {
}
@AfterClass
- public static void afterClass() {
- System.clearProperty("filterCache.enabled");
- System.clearProperty("queryResultCache.enabled");
- System.clearProperty("documentCache.enabled");
- System.clearProperty("solr.tests.id.stored");
- System.clearProperty("solr.tests.id.docValues");
- }
+ public static void afterClass() {}
Review Comment:
remove
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2672737704
##########
solr/core/src/test/org/apache/solr/cloud/LeaderFailureAfterFreshStartTest.java:
##########
@@ -93,78 +90,71 @@ public void test() throws Exception {
handle.clear();
handle.put("timestamp", SKIPVAL);
- try {
- CloudJettyRunner initialLeaderJetty = shardToLeaderJetty.get("shard1");
- List<CloudJettyRunner> otherJetties = getOtherAvailableJetties(initialLeaderJetty);
+ CloudJettyRunner initialLeaderJetty = shardToLeaderJetty.get("shard1");
+ List<CloudJettyRunner> otherJetties = getOtherAvailableJetties(initialLeaderJetty);
+ log.info(
+ "Leader node_name: {}, url: {}", initialLeaderJetty.coreNodeName, initialLeaderJetty.url);
+ for (CloudJettyRunner cloudJettyRunner : otherJetties) {
log.info(
- "Leader node_name: {}, url: {}",
- initialLeaderJetty.coreNodeName,
- initialLeaderJetty.url);
- for (CloudJettyRunner cloudJettyRunner : otherJetties) {
- log.info(
- "Non-leader node_name: {}, url: {}",
- cloudJettyRunner.coreNodeName,
- cloudJettyRunner.url);
- }
+ "Non-leader node_name: {}, url: {}",
+ cloudJettyRunner.coreNodeName,
+ cloudJettyRunner.url);
+ }
- CloudJettyRunner secondNode = otherJetties.get(0);
- CloudJettyRunner freshNode = otherJetties.get(1);
+ CloudJettyRunner secondNode = otherJetties.get(0);
+ CloudJettyRunner freshNode = otherJetties.get(1);
- // shutdown a node to simulate fresh start
- otherJetties.remove(freshNode);
- forceNodeFailures(singletonList(freshNode));
+ // shutdown a node to simulate fresh start
+ otherJetties.remove(freshNode);
+ forceNodeFailures(singletonList(freshNode));
- del("*:*");
- waitForThingsToLevelOut(30, TimeUnit.SECONDS);
+ del("*:*");
+ waitForThingsToLevelOut(30, TimeUnit.SECONDS);
- checkShardConsistency(false, true);
+ checkShardConsistency(false, true);
- // index a few docs and commit
- for (int i = 0; i < 100; i++) {
- indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
- }
- commit();
- waitForThingsToLevelOut(30, TimeUnit.SECONDS);
-
- checkShardConsistency(false, true);
-
- // bring down the other node and index a few docs; so the leader and other node segments
- // diverge
- forceNodeFailures(singletonList(secondNode));
- for (int i = 0; i < 10; i++) {
- indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
- if (i % 2 == 0) {
- commit();
- }
+ // index a few docs and commit
+ for (int i = 0; i < 100; i++) {
+ indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
+ }
+ commit();
+ waitForThingsToLevelOut(30, TimeUnit.SECONDS);
+
+ checkShardConsistency(false, true);
+
+ // bring down the other node and index a few docs; so the leader and other node segments
+ // diverge
+ forceNodeFailures(singletonList(secondNode));
+ for (int i = 0; i < 10; i++) {
+ indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
+ if (i % 2 == 0) {
+ commit();
}
- commit();
- restartNodes(singletonList(secondNode));
-
- // start the freshNode
- restartNodes(singletonList(freshNode));
- String coreName = freshNode.jetty.getCoreContainer().getCores().iterator().next().getName();
- Path replicationProperties =
- Path.of(
- freshNode.jetty.getSolrHome(), "cores", coreName, "data", "replication.properties");
- String md5 = DigestUtils.md5Hex(Files.readAllBytes(replicationProperties));
-
- // shutdown the original leader
- log.info("Now shutting down initial leader");
- forceNodeFailures(singletonList(initialLeaderJetty));
- waitForNewLeader(cloudClient, "shard1", initialLeaderJetty.info);
- waitTillNodesActive();
- log.info("Updating mappings from zk");
- updateMappingsFromZk(jettys, clients, true);
- assertEquals(
- "Node went into replication",
- md5,
- DigestUtils.md5Hex(Files.readAllBytes(replicationProperties)));
-
- success = true;
- } finally {
- System.clearProperty("solr.index.replication.fingerprint.enabled");
}
+ commit();
+ restartNodes(singletonList(secondNode));
+
+ // start the freshNode
+ restartNodes(singletonList(freshNode));
+ String coreName = freshNode.jetty.getCoreContainer().getCores().iterator().next().getName();
+ Path replicationProperties =
+ Path.of(freshNode.jetty.getSolrHome(), "cores", coreName, "data", "replication.properties");
+ String md5 = DigestUtils.md5Hex(Files.readAllBytes(replicationProperties));
+
+ // shutdown the original leader
+ log.info("Now shutting down initial leader");
+ forceNodeFailures(singletonList(initialLeaderJetty));
+ waitForNewLeader(cloudClient, "shard1", initialLeaderJetty.info);
+ waitTillNodesActive();
+ log.info("Updating mappings from zk");
+ updateMappingsFromZk(jettys, clients, true);
+ assertEquals(
+ "Node went into replication",
+ md5,
+ DigestUtils.md5Hex(Files.readAllBytes(replicationProperties)));
+
+ success = true;
Review Comment:
I *believe* that even if the test fails, our test infrastructure will reset the `solr.index.replication.fingerprint.enabled` property. We do not wrap our tests that set properties in try/catch brackets in general....
--
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] Remove unneeded clearing of system properties [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4010:
URL: https://github.com/apache/solr/pull/4010#discussion_r2677906249
##########
solr/core/src/test/org/apache/solr/cloud/TestCloudConsistency.java:
##########
@@ -59,6 +59,7 @@ public void setupCluster() throws Exception {
System.setProperty("solr.directoryFactory", "solr.StandardDirectoryFactory");
System.setProperty("solr.ulog.numRecordsToKeep", "1000");
System.setProperty("leaderVoteWait", "60000");
+ System.setProperty("tests.zk.violationReportAction", "IGNORE");
Review Comment:
after talking to @dsmiley going to take these "test fixes" out.
--
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