You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/10/27 15:57:42 UTC

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

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


##########
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) {
+      /* We're one datanode away from reaching the limit. Restrict potential
+      targets to targets that have already been selected.
+       */
+      findTargetStrategy.resetPotentialTargets(selectedTargets);
+      LOG.debug("Approaching max datanodes to involve limit. {} datanodes " +
+              "have already been selected for balancing and the limit is " +
+              "{}. Only already selected targets can be selected as targets" +
+              " now.",
+          countDatanodesInvolvedPerIteration, maxDatanodesToInvolve);
     }
-    if (sizeScheduledForMoveInLatestIteration +
-        (long) ozoneConfiguration.getStorageSize(
-        ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE,
-        ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT,
-        StorageUnit.BYTES) > maxSizeToMovePerIteration) {
-      if (LOG.isDebugEnabled()) {
-        LOG.debug("Hit max size to move limit. {} bytes have already been " +
-                "scheduled for balancing and the limit is {} bytes.",
-            sizeScheduledForMoveInLatestIteration,
-            maxSizeToMovePerIteration);
-      }
-      return true;
+
+    // we either didn't need to or were able to adapt, so return true
+    return true;
+  }
+
+  private boolean adaptOnReachingIterationLimits() {
+    // 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;
     }
-    return false;
+
+    // check if we've reached max datanodes to involve limit
+    int maxDatanodesToInvolve =
+        (int) (maxDatanodesRatioToInvolvePerIteration * totalNodesInCluster);
+    if (countDatanodesInvolvedPerIteration == maxDatanodesToInvolve) {

Review Comment:
   check should be >= as every iteration, as countDatanodesInvolvedPerIteration can be increased by 0-2 in range as per selection of source and target. 



##########
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:
   the check should involve " + 2 > " as range of increase can be 0-2 for source and target, eg, if max is 2, countDatanodeInvolved is also 2, it will fail



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