[GitHub] [ozone] siddhantsangwan commented on a change in pull request #2278: HDDS-4926. Support start/stop for container balancer via command line

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

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



##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java
##########
@@ -712,6 +718,73 @@ public boolean getReplicationManagerStatus() throws IOException {
 
   }
 
+  @Override
+  public boolean startContainerBalancer(Optional<Double> threshold,
+                  Optional<Integer> idleiterations,
+                  Optional<Integer> maxDatanodesToBalance,
+                  Optional<Long> maxSizeToMoveInGB) throws IOException{
+    StartContainerBalancerRequestProto.Builder builder =
+        StartContainerBalancerRequestProto.newBuilder();
+    builder.setTraceID(TracingUtil.exportCurrentSpan());
+
+    //make balancer configuration optional
+    if (threshold.isPresent()) {
+      double tsd = threshold.get();
+      Preconditions.checkState(tsd >= 0.0D && tsd < 1.0D,
+          "threshold should to be specified in range [0.0, 1.0).");
+      builder.setThreshold(tsd);
+    }
+    if (maxSizeToMoveInGB.isPresent()) {
+      long mstm = maxSizeToMoveInGB.get();
+      Preconditions.checkState(mstm > 0,
+          "maxSizeToMoveInGB must be positive.");
+      builder.setMaxSizeToMoveInGB(mstm);
+    }
+    if (maxDatanodesToBalance.isPresent()) {
+      int mdtb = maxDatanodesToBalance.get();
+      Preconditions.checkState(mdtb > 0,
+          "maxDatanodesToBalance must be positive.");
+      builder.setMaxDatanodesToBalance(mdtb);
+    }
+    if (idleiterations.isPresent()) {
+      int idi = idleiterations.get();
+      Preconditions.checkState(idi > 0 || idi == -1,
+          "maxDatanodesToBalance must be positive or" +

Review comment:
       Did you mean:
   ```suggestion
             "idleIterations must be positive or" +
   ```

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java
##########
@@ -78,6 +83,28 @@ public void setThreshold(double threshold) {
     this.threshold = String.valueOf(threshold);
   }
 
+  /**
+   * Gets the idle iteration value for Container Balancer.
+   *
+   * @return a idle iteration count larger than 0
+   */
+  public int getIdleIteration() {
+    return idleIterations;
+  }
+
+  /**
+   * Sets the idle iteration value for Container Balancer.
+   *
+   * @param count a idle iteration count larger than 0
+   */
+  public void setIdleIteration(int count) {
+    if (count < 1) {

Review comment:
       Aren't we also allowing `idleIterations` to be `-1`? In that case this condition needs to be changed.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -670,6 +674,65 @@ public boolean getReplicationManagerStatus() {
     return scm.getReplicationManager().isRunning();
   }
 
+  @Override
+  public boolean startContainerBalancer(Optional<Double> threshold,
+                  Optional<Integer> idleiterations,
+                  Optional<Integer> maxDatanodesToBalance,
+                  Optional<Long> maxSizeToMoveInGB) throws IOException{
+    getScm().checkAdminAccess(getRemoteUser());
+    ContainerBalancerConfiguration cbc = new ContainerBalancerConfiguration();
+    if (threshold.isPresent()) {
+      double tsd = threshold.get();
+      Preconditions.checkState(tsd >= 0.0D && tsd < 1.0D,
+          "threshold should to be specified in range [0.0, 1.0).");
+      cbc.setThreshold(tsd);
+    }
+    if (maxSizeToMoveInGB.isPresent()) {
+      long mstm = maxSizeToMoveInGB.get();
+      Preconditions.checkState(mstm > 0,
+          "maxSizeToMoveInGB must be positive.");
+      cbc.setMaxSizeToMove(mstm * OzoneConsts.GB);
+    }
+    if (maxDatanodesToBalance.isPresent()) {
+      int mdtb = maxDatanodesToBalance.get();
+      Preconditions.checkState(mdtb > 0,
+          "maxDatanodesToBalance must be positive.");
+      cbc.setMaxDatanodesToBalance(mdtb);
+    }
+    if (idleiterations.isPresent()) {
+      int idi = idleiterations.get();
+      Preconditions.checkState(idi > 0 || idi == -1,
+          "maxDatanodesToBalance must be positive or" +

Review comment:
       Did you mean:
   ```suggestion
             "idleIterations must be positive or" +
   ```

##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java
##########
@@ -134,11 +142,17 @@ public void unBalancedNodesListShouldBeEmptyWhenClusterIsBalanced() {
   @Test
   public void containerBalancerShouldStopWhenMaxDatanodesToBalanceIsReached() {
     balancerConfiguration.setMaxDatanodesToBalance(2);
-    balancerConfiguration.setThreshold(0);
+    balancerConfiguration.setThreshold(1);

Review comment:
       Why is `threshold` set to `1` here? A threshold of 1 means balancing is not required, which is incorrect for this test since we are checking the `maxDatanodesToBalance` configuration. Moreover threshold belongs to `[0, 1)`.




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