SOLR-17265: Fix randomized PRS testing #2436
Conversation
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java
Outdated
Show resolved
Hide resolved
| public static boolean isPRS() { | ||
| return PRS_DEFAULT_PROP == null | ||
| ? random().nextBoolean() |
There was a problem hiding this comment.
oh wow I see how this was the problem, leading to too-little control in consistency over wether it's enabled or not in practice
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java
Outdated
Show resolved
Hide resolved
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I'd like us to approach this in a general/patterned way; this isn't the only setting to do this for. Another is the distributed cluster processing booleans, which is already handled. See org.apache.solr.cloud.SolrCloudTestCase#configureCluster and MiniSolrCloudCluster.build
I feel both should be configured similarly (at the same places mostly).
| public static void setPrsDefault() { | ||
| Class<?> target = RandomizedTest.getContext().getTargetClass(); | ||
| if (target != null && target.isAnnotationPresent(NoPrs.class)) return; | ||
| if (isPRS()) { | ||
| System.setProperty("solr.prs.default", "true"); | ||
| String existingPrsDefault = EnvUtils.getProperty(PRS_DEFAULT_PROP); | ||
| String newPrsDefault; | ||
| if (target != null && target.isAnnotationPresent(NoPrs.class)) { | ||
| if (existingPrsDefault != null) { | ||
| System.setProperty(SAVED_PRS_DEFAULT_PROP, existingPrsDefault); | ||
| } | ||
| newPrsDefault = "false"; | ||
| } else if (existingPrsDefault != null) { | ||
| newPrsDefault = existingPrsDefault; | ||
| } else { | ||
| newPrsDefault = Boolean.toString(random().nextBoolean()); | ||
| } |
There was a problem hiding this comment.
Can't we instead rely on TestRuleRestoreSystemProperties, thus don't need to restore whatever the pre-existing sys prop (set at CLI?) is?
|
Thanks for the info on those. I've standardized both the reseting of sysProps as well as where the option is set from. |
| public static TestRule syspropRestore = | ||
| new TestRuleRestoreSystemProperties(NUMERIC_DOCVALUES_SYSPROP, PRS_DEFAULT_PROP); |
There was a problem hiding this comment.
AFAIK, this shouldn't be necessary because SolrTestCase (root of all Solr tests) already declares SystemPropertiesRestoreRule as a ClassRule and also STCJ4 has a test rule for this as well for test/method level restoration.
There was a problem hiding this comment.
Ahhh that makes sense. I'll remove the custom ones.
https://issues.apache.org/jira/browse/SOLR-17265
This fixes the test errors introduced by #2230
I'm not really certain what the previous logic was supposed to be doing, but
isPRS()practically returned a random true or false every time it was called, since the"use.per-replica"sysProp was never set.I've simplified the logic considerably. Before a class is run
"solr.prs.default"is set to eithertrueorfalse(either randomly, or determined by the Class annotation). Then ever call toisPRS()will read that sysProp to determine if the default is PRS or not.