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 2021/09/13 06:40:34 UTC
[GitHub] [ozone] siddhantsangwan opened a new pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
siddhantsangwan opened a new pull request #2631:
URL: https://github.com/apache/ozone/pull/2631
## What changes were proposed in this pull request?
ContainerBalancer incorrectly calculates `dataSizeBalancedGB` and `countDatanodesInvolvedPerIteration`. Datanodes involved are counted twice if they had been involved earlier.
This Jira fixes these bugs.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5733
## How was this patch tested?
`TestContainerBalancer` UT
--
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
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by 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
[GitHub] [ozone] lokeshj1703 merged pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by GitBox <gi...@apache.org>.
lokeshj1703 merged pull request #2631:
URL: https://github.com/apache/ozone/pull/2631
--
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
[GitHub] [ozone] siddhantsangwan commented on a change in pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2631:
URL: https://github.com/apache/ozone/pull/2631#discussion_r708873087
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -589,14 +595,23 @@ private boolean moveContainer(DatanodeDetails source,
Set<DatanodeDetails> selectedTargets,
ContainerMoveSelection moveSelection, DatanodeDetails source) {
countDatanodesInvolvedPerIteration += 2;
+ // don't count a source that has been involved in move earlier
+ if (sourceToTargetMap.containsKey(source) ||
+ selectedTargets.contains(source)) {
+ countDatanodesInvolvedPerIteration -= 1;
+ }
+ // don't count a target that has been involved in move earlier
+ if (selectedTargets.contains(moveSelection.getTargetNode())) {
+ countDatanodesInvolvedPerIteration -= 1;
+ }
Review comment:
The suggestion looks wrong semantically since we want to exclude re-counting a node that we have already counted earlier. For example in the suggested code, if both the if conditions are satisfied, the result will equal 2. But the correct result should be 0.
--
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
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2631:
URL: https://github.com/apache/ozone/pull/2631#discussion_r708985534
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -589,14 +595,23 @@ private boolean moveContainer(DatanodeDetails source,
Set<DatanodeDetails> selectedTargets,
ContainerMoveSelection moveSelection, DatanodeDetails source) {
countDatanodesInvolvedPerIteration += 2;
+ // don't count a source that has been involved in move earlier
+ if (sourceToTargetMap.containsKey(source) ||
+ selectedTargets.contains(source)) {
+ countDatanodesInvolvedPerIteration -= 1;
+ }
+ // don't count a target that has been involved in move earlier
+ if (selectedTargets.contains(moveSelection.getTargetNode())) {
+ countDatanodesInvolvedPerIteration -= 1;
+ }
Review comment:
sorry, i made a mistake here
```suggestion
// don't count a source that has been involved in move earlier
if (!sourceToTargetMap.containsKey(source) &&
!selectedTargets.contains(source)) {
countDatanodesInvolvedPerIteration++;
}
// don't count a target that has been involved in move earlier
if (!selectedTargets.contains(moveSelection.getTargetNode())) {
countDatanodesInvolvedPerIteration++;
}
```
--
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
[GitHub] [ozone] siddhantsangwan commented on a change in pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2631:
URL: https://github.com/apache/ozone/pull/2631#discussion_r709095160
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -589,14 +595,23 @@ private boolean moveContainer(DatanodeDetails source,
Set<DatanodeDetails> selectedTargets,
ContainerMoveSelection moveSelection, DatanodeDetails source) {
countDatanodesInvolvedPerIteration += 2;
+ // don't count a source that has been involved in move earlier
+ if (sourceToTargetMap.containsKey(source) ||
+ selectedTargets.contains(source)) {
+ countDatanodesInvolvedPerIteration -= 1;
+ }
+ // don't count a target that has been involved in move earlier
+ if (selectedTargets.contains(moveSelection.getTargetNode())) {
+ countDatanodesInvolvedPerIteration -= 1;
+ }
Review comment:
Done!
--
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
[GitHub] [ozone] siddhantsangwan commented on pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on pull request #2631:
URL: https://github.com/apache/ozone/pull/2631#issuecomment-918130452
@lokeshj1703 @JacksonYao287 please take a look!
--
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
[GitHub] [ozone] lokeshj1703 commented on a change in pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on a change in pull request #2631:
URL: https://github.com/apache/ozone/pull/2631#discussion_r723108031
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -314,9 +315,8 @@ private boolean initializeIteration() {
withinThresholdUtilizedNodes.add(datanodeUsageInfo);
}
}
- metrics.setDatanodesNumToBalance(new LongMetric(countDatanodesToBalance));
- // TODO update dataSizeToBalanceGB metric with overLoadedBytes and
- // underLoadedBytes
+ metrics.setDataSizeToBalanceGB(
+ Math.max(overUtilizedBytes, underUtilizedBytes));
Review comment:
Divide by GB.
--
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
[GitHub] [ozone] siddhantsangwan commented on a change in pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2631:
URL: https://github.com/apache/ozone/pull/2631#discussion_r721339504
##########
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:
Yes. Counting is performed both during an iteration (to check max datanodes and size per iteration limits) and at the end of an iteration (to get correct values once moves have actually been performed).
--
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
[GitHub] [ozone] lokeshj1703 commented on pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on pull request #2631:
URL: https://github.com/apache/ozone/pull/2631#issuecomment-938382666
@siddhantsangwan Thanks for the contribution! @JacksonYao287 Thanks for the review! I have committed the PR to master branch.
--
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
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2631:
URL: https://github.com/apache/ozone/pull/2631#discussion_r708985534
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -589,14 +595,23 @@ private boolean moveContainer(DatanodeDetails source,
Set<DatanodeDetails> selectedTargets,
ContainerMoveSelection moveSelection, DatanodeDetails source) {
countDatanodesInvolvedPerIteration += 2;
+ // don't count a source that has been involved in move earlier
+ if (sourceToTargetMap.containsKey(source) ||
+ selectedTargets.contains(source)) {
+ countDatanodesInvolvedPerIteration -= 1;
+ }
+ // don't count a target that has been involved in move earlier
+ if (selectedTargets.contains(moveSelection.getTargetNode())) {
+ countDatanodesInvolvedPerIteration -= 1;
+ }
Review comment:
sorry, i made a mistake here
```suggestion
// don't count a source that has been involved in move earlier
if (!sourceToTargetMap.containsKey(source) &&
!selectedTargets.contains(source)) {
countDatanodesInvolvedPerIteration++;
}
// don't count a target that has been involved in move earlier
if (&selectedTargets.contains(moveSelection.getTargetNode())) {
countDatanodesInvolvedPerIteration++;
}
```
--
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
[GitHub] [ozone] siddhantsangwan commented on a change in pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on a change in pull request #2631:
URL: https://github.com/apache/ozone/pull/2631#discussion_r721921219
##########
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:
This would involve a linear search to find the source dn for this `ContainerMoveSelection` every time. Should I implement it?
--
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
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2631: HDDS-5733. Incorrect calculation of iteration related metrics in ContainerBalancer
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2631:
URL: https://github.com/apache/ozone/pull/2631#discussion_r708383886
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -589,14 +595,23 @@ private boolean moveContainer(DatanodeDetails source,
Set<DatanodeDetails> selectedTargets,
ContainerMoveSelection moveSelection, DatanodeDetails source) {
countDatanodesInvolvedPerIteration += 2;
+ // don't count a source that has been involved in move earlier
+ if (sourceToTargetMap.containsKey(source) ||
+ selectedTargets.contains(source)) {
+ countDatanodesInvolvedPerIteration -= 1;
+ }
+ // don't count a target that has been involved in move earlier
+ if (selectedTargets.contains(moveSelection.getTargetNode())) {
+ countDatanodesInvolvedPerIteration -= 1;
+ }
Review comment:
```suggestion
// don't count a source that has been involved in move earlier
if (sourceToTargetMap.containsKey(source) ||
selectedTargets.contains(source)) {
countDatanodesInvolvedPerIteration++;
}
// don't count a target that has been involved in move earlier
if (selectedTargets.contains(moveSelection.getTargetNode())) {
countDatanodesInvolvedPerIteration++;
}
```
NIT
--
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