[GitHub] [solr] madrob commented on a change in pull request #644: SOLR-15996: disabling PerReplicaState was resulting in DOWN replicas

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

madrob commented on a change in pull request #644:
URL: https://github.com/apache/solr/pull/644#discussion_r807518186



##########
File path: solr/core/src/java/org/apache/solr/cloud/Overseer.java
##########
@@ -19,14 +19,7 @@
 import java.io.Closeable;
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.LinkedList;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
+import java.util.*;

Review comment:
       please configure your editor to not do this

##########
File path: solr/core/src/java/org/apache/solr/cloud/overseer/CollectionMutator.java
##########
@@ -160,6 +164,38 @@ public ZkWriteCommand modifyCollection(final ClusterState clusterState, ZkNodePr
     }
   }
 
+  /**
+   *  Update the state/leader of replicas in state.json from PRS data
+   */
+  public static DocCollection updateReplicasFromPrs(DocCollection coll, PerReplicaStates prs) {
+    //we are disabling PRS. Update the replica states
+    Map<String, Slice> modifiedSlices = new LinkedHashMap<>();
+    coll.forEachReplica((s, replica) -> {
+      PerReplicaStates.State prsState = prs.states.get(replica.getName());
+      if (prsState != null) {
+        if (prsState.state != replica.getState()) {

Review comment:
       This if statement and the next one look like they are almost doing the same thing, is there a way to simplify this logic so that we are only getting/setting from modifiedSlices once?

##########
File path: solr/core/src/java/org/apache/solr/cloud/overseer/CollectionMutator.java
##########
@@ -55,7 +55,8 @@ public ZkWriteCommand createShard(final ClusterState clusterState, ZkNodeProps m
     DocCollection collection = clusterState.getCollection(collectionName);
     Slice slice = collection.getSlice(shardId);
     if (slice == null) {
-      Map<String, Replica> replicas = Collections.emptyMap();
+      @SuppressWarnings({"unchecked"})
+      Map<String, Replica> replicas = Collections.EMPTY_MAP;

Review comment:
       why?

##########
File path: solr/core/src/java/org/apache/solr/cloud/overseer/CollectionMutator.java
##########
@@ -113,10 +114,13 @@ public ZkWriteCommand modifyCollection(final ClusterState clusterState, ZkNodePr
           log.error("trying to set perReplicaState to {} from {}", val, coll.isPerReplicaState());
           continue;
         }
-        replicaOps = PerReplicaStatesOps.modifyCollection(coll, enable, PerReplicaStates.fetch(coll.getZNode(), zkClient, null));
+        PerReplicaStates prs = PerReplicaStates.fetch(coll.getZNode(), zkClient, null);
+        replicaOps = PerReplicaStatesOps.modifyCollection(coll, enable, prs);
+        if(!enable) {
+          coll = updateReplicasFromPrs(coll, prs);
+        }

Review comment:
       Is there still a race condition here? What happens if a replica goes down (or changes leadership) between the fetch and the persist which happens much later?




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