Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable PRS for all cloud tests #2199

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

noblepaul
Copy link
Contributor

WIP

@magibney
Copy link
Contributor

Should we instead have this default at the lowest level to being randomized, or otherwise ensure that nothing (either now or future) falls through the cracks?

This would yield a much smaller diff, but more importantly: having the randomized/configurable default at a lower level would improve confidence that we're achieving the goal of test coverage for both PRS and non-PRS case. Ideally it'd be nice to only explicitly specify CollectionAdminRequest.setPerReplicaState(boolean) when a test is actually testing behavior that relies specifically on PRS or non-PRS.

Specifically, I'm wondering about changing this line:

--- a/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
@@ -684,7 +684,7 @@ public abstract class CollectionAdminRequest<T extends CollectionAdminResponse>
         addProperties(params, properties);
       }
       numReplicas.writeProps(params);
-      if (Boolean.TRUE.equals(perReplicaState)) {
+      if (perReplicaState == null ? DEFAULT_PRS : Boolean.TRUE.equals(perReplicaState)) {
         params.set(CollectionAdminParams.PER_REPLICA_STATE, perReplicaState);
       }
       params.setNonNull(ALIAS, alias);

... where DEFAULT_PRS would be a new boolean constant (or pseudo-constant) initialized via sysprop (analogous to current SolrCloudTestCase.USE_PER_REPLICA_STATE).

@noblepaul
Copy link
Contributor Author

noblepaul commented Jan 22, 2024

Should we instead have this default at the lowest level to being randomized, or otherwise ensure that nothing (either now or future) falls through the cracks?

I think we should explore that. We do have tests that do PRS on and off explicitly. It'll be better if we do something in the test framework level instead of SolrJ

@hiteshk25
Copy link
Contributor

do we log somewhere whether collection has prs(or not)?

Copy link

This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the dev@solr.apache.org mailing list. Thank you for your contribution!

@github-actions github-actions bot added the stale PR not updated in 60 days label Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR not updated in 60 days
Projects
None yet
3 participants