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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
