[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #3895: HDDS-5526. ContainerBalancer#checkConditionsForBalancing pre-emptively checks iteration limits.

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

siddhantsangwan commented on code in PR #3895:
URL: https://github.com/apache/ozone/pull/3895#discussion_r1008091260


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -663,37 +680,69 @@ private ContainerMoveSelection matchSourceWithTarget(DatanodeDetails source) {
     return moveSelection;
   }
 
-  /**
-   * Checks if limits maxDatanodesPercentageToInvolvePerIteration and
-   * maxSizeToMovePerIteration have been hit.
-   *
-   * @return true if a limit was hit, else false
-   */
-  private boolean checkIterationLimits() {
+  private boolean reachedMaxSizeToMovePerIteration() {
+    // since candidate containers in ContainerBalancerSelectionCriteria are
+    // filtered out according to this limit, balancer should not have crossed it
+    if (sizeScheduledForMoveInLatestIteration >= maxSizeToMovePerIteration) {
+      LOG.warn("Reached max size to move limit. {} bytes have already been" +
+              " scheduled for balancing and the limit is {} bytes.",
+          sizeScheduledForMoveInLatestIteration, maxSizeToMovePerIteration);
+      return true;
+    }
+    return false;
+  }
+
+  private boolean adaptWhenNearingIterationLimits() {
+    // check if we've reached max size to move limit
+    if (reachedMaxSizeToMovePerIteration()) {
+      // return false because we cannot adapt if we've reached max size to
+      // move limit
+      return false;
+    }
+
+    // check if we've nearing max datanodes to involve
     int maxDatanodesToInvolve =
         (int) (maxDatanodesRatioToInvolvePerIteration * totalNodesInCluster);
-    if (countDatanodesInvolvedPerIteration + 2 > maxDatanodesToInvolve) {
-      if (LOG.isDebugEnabled()) {
-        LOG.debug("Hit max datanodes to involve limit. {} datanodes have" +
-                " already been scheduled for balancing and the limit is {}.",
-            countDatanodesInvolvedPerIteration, maxDatanodesToInvolve);
-      }
-      return true;
+    if (countDatanodesInvolvedPerIteration + 1 == maxDatanodesToInvolve) {

Review Comment:
   Yes, the range is 0-2.
   
   - If we're 2 datanodes away from the limit, we don't need to take any action.
   - This check is intended for the case when we're 1 datanode away from the limit. In this case we can allow only one new datanode at max to be selected. So, we restrict potential sources - that means only a new target can be selected now.
   - In your example where max is 2 and count is 2 (we've reached the limit), yes this check will fail but the check in `adaptOnReachingIterationLimits` will pass. It will restrict both sources and targets, so no new datanodes will be selected.
   
   Does this make sense or have I misunderstood your 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.

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