cpoerschke commented on code in PR #924:
URL: https://github.com/apache/solr/pull/924#discussion_r912746051
##########
solr/core/src/test/org/apache/solr/cloud/OverseerCollectionConfigSetProcessorTest.java:
##########
@@ -103,6 +103,7 @@ public class OverseerCollectionConfigSetProcessorTest
extends SolrTestCaseJ4 {
private static final String ADMIN_PATH = "/admin/cores";
private static final String COLLECTION_NAME = "mycollection";
private static final String CONFIG_NAME = "myconfig";
+ public static final int MAXWAIT = 10000;
Review Comment:
```suggestion
private static final int MAX_WAIT_MS = 10000;
```
##########
solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java:
##########
@@ -77,6 +77,7 @@
@LuceneTestCase.Slow
public class CollectionsAPISolrJTest extends SolrCloudTestCase {
+ public static final int TIMEOUT = 3000;
Review Comment:
```suggestion
private static final int TIMEOUT = 3000;
```
##########
solr/core/src/test/org/apache/solr/cloud/PeerSyncReplicationTest.java:
##########
@@ -289,15 +288,13 @@ private CloudJettyRunner
forceNodeFailureAndDoPeerSync(boolean disableFingerprin
indexDoc(id, docId, i1, 50, tlong, 50, t1, "document number " + docId++);
commit();
- bringUpDeadNodeAndEnsureNoReplication(replicaToShutDown,
disableFingerprint);
+ bringUpDeadNodeAndEnsureNoReplication(replicaToShutDown);
return replicaToShutDown;
}
- private void bringUpDeadNodeAndEnsureNoReplication(
- CloudJettyRunner nodeToBringUp, boolean disableFingerprint) throws
Exception {
- // disable fingerprint check if needed
- System.setProperty("solr.disableFingerprint",
String.valueOf(disableFingerprint));
Review Comment:
This looks like a logic change? Suggest to defer i.e. to out-scope from this
code cleanup round.
##########
solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java:
##########
@@ -210,7 +210,8 @@ public void testIntersectFilter() throws Exception {
public void checkResultFormat() {
// Check input and output format is the same
String IN = "89.9,-130"; // lat,lon
Review Comment:
How about combining the two variables?
```suggestion
final String IN_OUT = "89.9,-130"; // lat,lon
```
##########
solr/core/src/test/org/apache/solr/handler/component/StatsComponentTest.java:
##########
@@ -1248,17 +1248,16 @@ public void testMiscQueryStats() throws Exception {
// force constant score for matches, so we aren't dependent on similarity
final float constScore = 4.2F;
- final double expectedScore = (double) constScore;
Review Comment:
subjective: clearer before IMO i.e. fewer casts and clear that it's the
expected score
##########
solr/core/src/test/org/apache/solr/schema/TestCloudSchemaless.java:
##########
@@ -63,7 +63,8 @@ protected String getCloudSolrConfig() {
@Override
public SortedMap<ServletHolder, String> getExtraServlets() {
- final SortedMap<ServletHolder, String> extraServlets = new TreeMap<>();
Review Comment:
I think I'd prefer how it was before, but that's subjective. Separately (and
out of the scope of this pull request) wondering if the `getExtraServlets`
stuff within the code base is still needed or if it can be removed perhaps.
##########
solr/core/src/test/org/apache/solr/cloud/OverseerCollectionConfigSetProcessorTest.java:
##########
@@ -894,10 +895,10 @@ protected void verifySubmitCaptures(
}
}
- protected void waitForEmptyQueue(long maxWait) throws Exception {
- final TimeOut timeout = new TimeOut(maxWait, TimeUnit.MILLISECONDS,
TimeSource.NANO_TIME);
+ protected void waitForEmptyQueue() throws Exception {
+ final TimeOut timeout = new TimeOut(MAXWAIT, TimeUnit.MILLISECONDS,
TimeSource.NANO_TIME);
while (queue.peek() != null) {
- if (timeout.hasTimedOut()) fail("Queue not empty within " + maxWait + "
ms");
+ if (timeout.hasTimedOut()) fail("Queue not empty within " + (long) 10000
+ " ms");
Review Comment:
```suggestion
if (timeout.hasTimedOut()) fail("Queue not empty within " +
MAX_WAIT_MS + " ms");
```
##########
solr/core/src/test/org/apache/solr/cloud/CollectionPropsTest.java:
##########
@@ -45,6 +45,7 @@
@LuceneTestCase.Slow
@SolrTestCaseJ4.SuppressSSL
public class CollectionPropsTest extends SolrCloudTestCase {
+ public static final int TIMEOUT = 5000;
Review Comment:
might `private` be sufficient?
```suggestion
private static final int TIMEOUT = 5000;
```
##########
solr/core/src/test/org/apache/solr/update/processor/UUIDUpdateProcessorFallbackTest.java:
##########
@@ -165,7 +165,7 @@ SolrInputField field(String name, float boost, Object...
values) {
/** Convenience method for building up SolrInputFields with default boost */
Review Comment:
```suggestion
/** Convenience method for building up SolrInputFields */
```
Does this mean then that `f` can be removed in favour of `field` actually?
##########
solr/core/src/test/org/apache/solr/cloud/ReindexCollectionTest.java:
##########
@@ -167,24 +167,23 @@ public void testSameTargetReindexing() throws Exception {
private void doTestSameTargetReindexing(boolean sourceRemove, boolean
followAliases)
throws Exception {
- final String sourceCollection = "sameTargetReindexing_" + sourceRemove +
"_" + followAliases;
- final String targetCollection = sourceCollection;
Review Comment:
source vs. target collection naming seems helpful here w.r.t. test logic
readability
##########
solr/core/src/test/org/apache/solr/handler/component/ShardsAllowListTest.java:
##########
@@ -85,7 +85,8 @@ public static void setupClusters() throws Exception {
@Override
public MiniSolrCloudCluster apply(String clusterId) {
try {
- final MiniSolrCloudCluster cluster =
+ final MiniSolrCloudCluster cluster;
+ cluster =
Review Comment:
```suggestion
return
```
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
