[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer

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

lokeshj1703 commented on a change in pull request #2631:
URL: https://github.com/apache/ozone/pull/2631#discussion_r710034959



##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -426,14 +427,15 @@ private IterationResult doIteration() {
             ContainerInfo container =
                 containerManager.getContainer(moveSelection.getContainerID());
             this.sizeMovedPerIteration += container.getUsedBytes();
-            this.countDatanodesInvolvedPerIteration += 2;
+            metrics.incrementMovedContainersNum(1);
+            LOG.info("Move completed for container {} to target {}",

Review comment:
       Maybe we should also log the source dn here.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
##########
@@ -32,11 +32,11 @@
 
   @Metric(about = "The total amount of used space in GigaBytes that needs to " +
       "be balanced.")
-  private LongMetric dataSizeToBalanceGB;
+  private double dataSizeToBalanceGB;

Review comment:
       We might need to create a DoubleMetric. JsonAutoDetect seems to be used for visibility in LongMetric.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java
##########
@@ -32,11 +32,11 @@
 
   @Metric(about = "The total amount of used space in GigaBytes that needs to " +
       "be balanced.")
-  private LongMetric dataSizeToBalanceGB;
+  private double dataSizeToBalanceGB;

Review comment:
       Let's also check if double value is supported. We are using org.apache.hadoop.metrics2.annotation.Metric.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -588,15 +594,23 @@ private boolean moveContainer(DatanodeDetails source,
       Collection<DatanodeDetails> potentialTargets,
       Set<DatanodeDetails> selectedTargets,
       ContainerMoveSelection moveSelection, DatanodeDetails source) {
-    countDatanodesInvolvedPerIteration += 2;
+    // count source if it has not been involved in move earlier
+    if (!sourceToTargetMap.containsKey(source) &&
+        !selectedTargets.contains(source)) {
+      countDatanodesInvolvedPerIteration += 1;
+    }
+    // count target if it has not been involved in move earlier
+    if (!selectedTargets.contains(moveSelection.getTargetNode())) {
+      countDatanodesInvolvedPerIteration += 1;
+    }

Review comment:
       Do we need this since we are already setting this metric above?




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