[GitHub] [ozone] ashishkumar50 commented on a diff in pull request #5097: HDDS-9011. EC: Unhealthy replica not replaced with a healthy replica in a rack-scatter environment

["ashishkumar50 (via GitHub)" <gi...@apache.org>]

ashishkumar50 commented on code in PR #5097:
URL: https://github.com/apache/ozone/pull/5097#discussion_r1269591382


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java:
##########
@@ -317,31 +362,48 @@ protected List<DatanodeDetails> chooseDatanodesInternal(
                       "available racks. {} Available racks count: {},"
                       + " Excluded nodes count: {}, UsedNodes count: {}",
               reason, racks.size(), excludedNodesCount, usedNodesCount);
-      throw new SCMException(reason,
-              SCMException.ResultCodes.FAILED_TO_FIND_HEALTHY_NODES);
+
+      /*
+      Couldn't choose enough racks to satisfy the rack scatter policy. If
+      fallback is not allowed, then throw an exception.
+      */
+      if (!fallback) {
+        throw new SCMException(reason,
+            SCMException.ResultCodes.FAILED_TO_FIND_HEALTHY_NODES);
+      }
     }
 
     if (chosenNodes.size() < nodesRequired) {
+      /* If fallback is allowed, adjust maxReplicasToMove such that
+      replication factor number of replicas are allowed per rack.
+      */
+      if (fallback) {
+        maxReplicasPerRack = requiredReplicationFactor;
+      }
       racks.addAll(usedRacksCntMap.keySet());
       racks = sortRackWithExcludedNodes(racks, excludedNodes, usedRacksCntMap);
       racks.addAll(usedRacksCntMap.keySet());
+      LOG.debug("Available racks considering racks with used and exclude " +
+              "nodes: {}.", racks);
       chosenNodes.addAll(chooseNodesFromRacks(racks, unavailableNodes,
               mutableFavoredNodes, nodesRequired - chosenNodes.size(),
               metadataSizeRequired, dataSizeRequired,
-              Integer.MAX_VALUE, usedRacksCntMap, maxReplicasPerRack));
+              nodesRequired - chosenNodes.size(),

Review Comment:
   Changing `maxOuterLoopIterations` to exact number of nodes required, This will decrease probablity of getting exact number of nodes required.
   [chooseRandom](https://github.com/apache/ozone/blob/cd31fd57efa4f86d45b45aa8ad9e7e2eac1ddf3b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java#L394) will return any random node. There is chance that random node is not valid [isValidNode](https://github.com/apache/ozone/blob/cd31fd57efa4f86d45b45aa8ad9e7e2eac1ddf3b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java#L405).
   After certain retry(currently its 5) it will return null node back to `chooseNodesFromRacks`.
   Assuming this happens for all the racks. Then at the end of the loop we decrement `maxOuterLoopIterations` without even choosing any node. So the probability of getting required nodes keeps decreasing.
   I think maxOuterLoopIterations should not bound to number of nodes required.



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