You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "HoustonPutman (via GitHub)" <gi...@apache.org> on 2024/05/01 18:49:42 UTC
[PR] SOLR-17265: Fix randomized PRS testing [solr]
HoustonPutman opened a new pull request, #2436:
URL: https://github.com/apache/solr/pull/2436
https://issues.apache.org/jira/browse/SOLR-17265
This fixes the test errors introduced by https://github.com/apache/solr/pull/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 either `true` or `false` (either randomly, or determined by the Class annotation). Then ever call to `isPRS()` will read that sysProp to determine if the default is PRS or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17265: Fix randomized PRS testing [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #2436:
URL: https://github.com/apache/solr/pull/2436#discussion_r1587879478
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractBasicDistributedZk2TestBase.java:
##########
@@ -56,6 +58,10 @@ public abstract class AbstractBasicDistributedZk2TestBase extends AbstractFullDi
private static final String ONE_NODE_COLLECTION = "onenodecollection";
private final boolean onlyLeaderIndexes = random().nextBoolean();
+ @ClassRule
+ public static TestRule syspropRestore =
+ new TestRuleRestoreSystemProperties(NUMERIC_DOCVALUES_SYSPROP, PRS_DEFAULT_PROP);
Review Comment:
Ahhh that makes sense. I'll remove the custom ones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17265: Fix randomized PRS testing [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman merged PR #2436:
URL: https://github.com/apache/solr/pull/2436
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17265: Fix randomized PRS testing [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #2436:
URL: https://github.com/apache/solr/pull/2436#discussion_r1586713299
##########
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java:
##########
@@ -140,29 +138,18 @@ public static void shutdownCluster() throws Exception {
@BeforeClass
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");
+ boolean prsDefault;
+ if (target != null && target.isAnnotationPresent(NoPrs.class)) {
+ prsDefault = false;
+ } else {
+ prsDefault = random().nextBoolean();
Review Comment:
Done, and added another sysProp to save the user-provided value that we might be overwriting because of Class annotations
##########
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java:
##########
@@ -83,7 +83,7 @@ public class SolrCloudTestCase extends SolrTestCaseJ4 {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- public static final String PRS_DEFAULT_PROP = System.getProperty("use.per-replica", null);
+ public static final String PRS_DEFAULT_PROP = "solr.prs.default";
Review Comment:
Put it in Solr where it is used.
##########
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java:
##########
@@ -97,11 +97,9 @@ protected static SolrZkClient zkClient() {
return cluster.getZkStateReader().getZkClient();
}
- /** if the system property is not specified, use a random value */
+ /** if the system property is not specified, default to false. The SystemProperty will be set in a beforeClass method. */
public static boolean isPRS() {
- return PRS_DEFAULT_PROP == null
- ? random().nextBoolean()
- : Boolean.parseBoolean(PRS_DEFAULT_PROP);
+ return Boolean.parseBoolean(System.getProperty(PRS_DEFAULT_PROP, "false"));
Review Comment:
Used it, in all places, but really in the tests, it will only be cared about in the `setPrsDefault()` since we overwrite the sysProp always.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17265: Fix randomized PRS testing [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #2436:
URL: https://github.com/apache/solr/pull/2436#issuecomment-2089162157
Thanks for the info on those. I've standardized both the reseting of sysProps as well as where the option is set from.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17265: Fix randomized PRS testing [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2436:
URL: https://github.com/apache/solr/pull/2436#discussion_r1586911809
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractBasicDistributedZk2TestBase.java:
##########
@@ -56,6 +58,10 @@ public abstract class AbstractBasicDistributedZk2TestBase extends AbstractFullDi
private static final String ONE_NODE_COLLECTION = "onenodecollection";
private final boolean onlyLeaderIndexes = random().nextBoolean();
+ @ClassRule
+ public static TestRule syspropRestore =
+ new TestRuleRestoreSystemProperties(NUMERIC_DOCVALUES_SYSPROP, PRS_DEFAULT_PROP);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17265: Fix randomized PRS testing [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2436:
URL: https://github.com/apache/solr/pull/2436#discussion_r1586786180
##########
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java:
##########
@@ -140,28 +143,29 @@ public static void shutdownCluster() throws Exception {
@BeforeClass
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());
}
Review Comment:
Can't we instead rely on TestRuleRestoreSystemProperties, thus don't need to restore whatever the pre-existing sys prop (set at CLI?) is?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17265: Fix randomized PRS testing [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2436:
URL: https://github.com/apache/solr/pull/2436#discussion_r1586911809
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractBasicDistributedZk2TestBase.java:
##########
@@ -56,6 +58,10 @@ public abstract class AbstractBasicDistributedZk2TestBase extends AbstractFullDi
private static final String ONE_NODE_COLLECTION = "onenodecollection";
private final boolean onlyLeaderIndexes = random().nextBoolean();
+ @ClassRule
+ public static TestRule syspropRestore =
+ new TestRuleRestoreSystemProperties(NUMERIC_DOCVALUES_SYSPROP, PRS_DEFAULT_PROP);
Review 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. Even LuceneTestCase adds a ClassRule for TestRuleRestoreSystemProperties, thus STC's is maybe redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17265: Fix randomized PRS testing [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2436:
URL: https://github.com/apache/solr/pull/2436#discussion_r1586652988
##########
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java:
##########
@@ -83,7 +83,7 @@ public class SolrCloudTestCase extends SolrTestCaseJ4 {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- public static final String PRS_DEFAULT_PROP = System.getProperty("use.per-replica", null);
+ public static final String PRS_DEFAULT_PROP = "solr.prs.default";
Review Comment:
Out of scope for now but but I'd like such a setting to exist Solr side officially
##########
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java:
##########
@@ -97,11 +97,9 @@ protected static SolrZkClient zkClient() {
return cluster.getZkStateReader().getZkClient();
}
- /** if the system property is not specified, use a random value */
+ /** if the system property is not specified, default to false. The SystemProperty will be set in a beforeClass method. */
public static boolean isPRS() {
- return PRS_DEFAULT_PROP == null
- ? random().nextBoolean()
- : Boolean.parseBoolean(PRS_DEFAULT_PROP);
+ return Boolean.parseBoolean(System.getProperty(PRS_DEFAULT_PROP, "false"));
Review Comment:
Can EnvUtils be used here?
##########
solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java:
##########
@@ -97,11 +97,9 @@ protected static SolrZkClient zkClient() {
return cluster.getZkStateReader().getZkClient();
}
- /** if the system property is not specified, use a random value */
+ /** if the system property is not specified, default to false. The SystemProperty will be set in a beforeClass method. */
public static boolean isPRS() {
- return PRS_DEFAULT_PROP == null
- ? random().nextBoolean()
Review 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:
##########
@@ -140,29 +138,18 @@ public static void shutdownCluster() throws Exception {
@BeforeClass
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");
+ boolean prsDefault;
+ if (target != null && target.isAnnotationPresent(NoPrs.class)) {
+ prsDefault = false;
+ } else {
+ prsDefault = random().nextBoolean();
Review Comment:
shouldn't this **read** a sys prop so we can choose it for the build?
--
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