[GitHub] [hadoop-ozone] nandakumar131 commented on a change in pull request #1423: HDDS-4244. Container deleted wrong replica cause mis-replicated.

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

nandakumar131 commented on a change in pull request #1423:
URL: https://github.com/apache/hadoop-ozone/pull/1423#discussion_r491900009



##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
##########
@@ -660,21 +660,23 @@ private void handleOverReplicatedContainer(final ContainerInfo container,
       if (excess > 0) {
         eligibleReplicas.removeAll(unhealthyReplicas);
         Set<ContainerReplica> replicaSet = new HashSet<>(eligibleReplicas);
-        boolean misReplicated =
-            getPlacementStatus(replicaSet, replicationFactor)
-                .isPolicySatisfied();
+        ContainerPlacementStatus ps =
+            getPlacementStatus(replicaSet, replicationFactor);
         for (ContainerReplica r : eligibleReplicas) {
           if (excess <= 0) {
             break;
           }
           // First remove the replica we are working on from the set, and then
           // check if the set is now mis-replicated.
           replicaSet.remove(r);
-          boolean nowMisRep = getPlacementStatus(replicaSet, replicationFactor)
-              .isPolicySatisfied();
-          if (misReplicated || !nowMisRep) {
-            // Remove the replica if the container was already mis-replicated
-            // OR if losing this replica does not make it become mis-replicated
+          ContainerPlacementStatus nowPS =
+              getPlacementStatus(replicaSet, replicationFactor);
+          if ((!ps.isPolicySatisfied()
+                && nowPS.actualPlacementCount() == ps.actualPlacementCount())
+              || (ps.isPolicySatisfied() && nowPS.isPolicySatisfied())) {

Review comment:
       Why do we need such a complex condition check?
   Won't fixing the previous check solve the problem?
   ```if (misReplicated || !nowMisRep)``` to ```if (!misReplicated || nowMisRep)```




----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org