[GitHub] [lucene-solr] murblanc commented on a change in pull request #2318: SOLR-15138: PerReplicaStates does not scale to large collections as well as state.json

[GitBox <gi...@apache.org>]

murblanc commented on a change in pull request #2318:
URL: https://github.com/apache/lucene-solr/pull/2318#discussion_r572075066



##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/PerReplicaStates.java
##########
@@ -92,6 +94,17 @@ public PerReplicaStates(String path, int cversion, List<String> states) {
 
   }
 
+  /** Check and return if all replicas are ACTIVE
+   */
+  public boolean allActive() {
+    if (this.allActive != null) return allActive;
+    boolean[] result = new boolean[]{true};

Review comment:
       `AtomicBoolean` would likely make code easier to read.

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -264,8 +264,11 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
         log.info("Cleaned up artifacts for failed create collection for [{}]", collectionName);
         throw new SolrException(ErrorCode.BAD_REQUEST, "Underlying core creation failed while creating collection: " + collectionName);
       } else {
+        //we want to wait till all the replicas are ACTIVE for PRS collections because
+          ocmh.zkStateReader.waitForState(collectionName, 30, TimeUnit.SECONDS, (liveNodes, c) ->
+                  c.getPerReplicaStates() == null || // this is not a PRS collection

Review comment:
       Shouldn't that test about collection not being PRS be made before the call to `waitForState` to just skip that call?

##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/PerReplicaStates.java
##########
@@ -92,6 +94,17 @@ public PerReplicaStates(String path, int cversion, List<String> states) {
 
   }
 
+  /** Check and return if all replicas are ACTIVE
+   */
+  public boolean allActive() {
+    if (this.allActive != null) return allActive;

Review comment:
       This method caches the value computed the first time it's called, then returns that value on each subsequent call.
   When used as part of the predicate in `CollectionStateWatcher`, even if the watch fires multiple times wouldn't it always return the first computed state (which is likely `false` as all replicas are not active initially)?

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -264,8 +264,11 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
         log.info("Cleaned up artifacts for failed create collection for [{}]", collectionName);
         throw new SolrException(ErrorCode.BAD_REQUEST, "Underlying core creation failed while creating collection: " + collectionName);
       } else {
+        //we want to wait till all the replicas are ACTIVE for PRS collections because

Review comment:
       Truncated comment




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org