[GitHub] [ozone] siddhantsangwan commented on a change in pull request #2349: HDDS-4928. Support container move in Replication Manager

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

siddhantsangwan commented on a change in pull request #2349:
URL: https://github.com/apache/ozone/pull/2349#discussion_r664238672



##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
##########
@@ -862,43 +1153,128 @@ private void handleOverReplicatedContainer(final ContainerInfo container,
           break;
         }
       }
-      // After removing all unhealthy replicas, if the container is still over
-      // replicated then we need to check if it is already mis-replicated.
-      // If it is, we do no harm by removing excess replicas. However, if it is
-      // not mis-replicated, then we can only remove replicas if they don't
-      // make the container become mis-replicated.
-      if (excess > 0) {
-        eligibleReplicas.removeAll(unhealthyReplicas);
-        Set<ContainerReplica> eligibleSet = new HashSet<>(eligibleReplicas);
-        ContainerPlacementStatus ps =
-            getPlacementStatus(eligibleSet, replicationFactor);
-        for (ContainerReplica r : eligibleReplicas) {
-          if (excess <= 0) {
+
+      eligibleReplicas.removeAll(unhealthyReplicas);
+      boolean isInMove = inflightMove.containsKey(id);
+      boolean isSourceDnInReplicaSet = false;
+      boolean isTargetDnInReplicaSet = false;
+
+      if (isInMove) {
+        Pair<DatanodeDetails, DatanodeDetails> movePair =
+            inflightMove.get(id);
+        final DatanodeDetails sourceDN = movePair.getKey();
+        isSourceDnInReplicaSet = eligibleReplicas.stream()
+            .anyMatch(r -> r.getDatanodeDetails().equals(sourceDN));
+        isTargetDnInReplicaSet = eligibleReplicas.stream()
+            .anyMatch(r -> r.getDatanodeDetails()
+                .equals(movePair.getValue()));
+        int sourceDnPos = 0;
+        for (int i = 0; i < eligibleReplicas.size(); i++) {
+          if (eligibleReplicas.get(i).getDatanodeDetails()
+              .equals(sourceDN)) {
+            sourceDnPos = i;
             break;
           }
-          // First remove the replica we are working on from the set, and then
-          // check if the set is now mis-replicated.
-          eligibleSet.remove(r);
-          ContainerPlacementStatus nowPS =
-              getPlacementStatus(eligibleSet, replicationFactor);
-          if ((!ps.isPolicySatisfied()
-                && nowPS.actualPlacementCount() == ps.actualPlacementCount())
-              || (ps.isPolicySatisfied() && nowPS.isPolicySatisfied())) {
-            // Remove the replica if the container was already unsatisfied
-            // and losing this replica keep actual placement count unchanged.
-            // OR if losing this replica still keep satisfied
-            sendDeleteCommand(container, r.getDatanodeDetails(), true);
-            excess -= 1;
-            continue;
+        }
+        if (isTargetDnInReplicaSet) {
+          if (isSourceDnInReplicaSet) {
+            // if the target is present, and source disappears somehow,
+            // we can consider move is successful.
+            inflightMoveFuture.get(id).complete(MoveResult.COMPLETED);
+            inflightMove.remove(id);
+            inflightMoveFuture.remove(id);
+          } else {
+            // if the container is in inflightMove and target datanode is
+            // included in the replicas, then swap the source datanode to
+            // first of the replica list if exists, so the source datanode
+            // will be first removed if possible.
+            eligibleReplicas.add(0, eligibleReplicas.remove(sourceDnPos));
+          }

Review comment:
       Can you please explain this part? If both target and source are present in replicas, then source should be swapped with the first node in replicas list so it can be deleted, right? And move completes if source is absent while target is present. It seems the logic of if and else parts is opposite.




-- 
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@ozone.apache.org

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



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