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/02/24 02:11:11 UTC
[GitHub] [ozone] symious commented on a change in pull request #3129: HDDS-6367. ContainerBalancer shows incorrect iteration result sometimes
symious commented on a change in pull request #3129:
URL: https://github.com/apache/ozone/pull/3129#discussion_r813486442
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -417,24 +425,32 @@ private IterationResult doIteration() {
findSourceStrategy.removeCandidateSourceDataNode(source);
}
}
-
- if (!isMoveGenerated) {
- //no move option is generated, so the cluster can not be
- //balanced any more, just stop iteration and exit
- return IterationResult.CAN_NOT_BALANCE_ANY_MORE;
- }
- return IterationResult.ITERATION_COMPLETED;
} finally {
- checkIterationMoveResults(selectedTargets);
Review comment:
I think finally block better fits the idea of cleaning code, the new version seems to have too much process logic in it. Personally, I do like the original method.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
##########
@@ -375,26 +378,31 @@ private IterationResult doIteration() {
findSourceStrategy.reInitialize(getPotentialSources(), config, lowerLimit);
List<DatanodeUsageInfo> potentialTargets = getPotentialTargets();
findTargetStrategy.reInitialize(potentialTargets, config, upperLimit);
-
Set<DatanodeDetails> selectedTargets =
new HashSet<>(potentialTargets.size());
moveSelectionToFutureMap = new HashMap<>(unBalancedNodes.size());
boolean isMoveGenerated = false;
+ iterationResult = IterationResult.ITERATION_COMPLETED;
+
try {
// match each overUtilized node with a target
while (true) {
if (!isBalancerRunning()) {
- return IterationResult.ITERATION_INTERRUPTED;
+ iterationResult = IterationResult.ITERATION_INTERRUPTED;
+ break;
}
- IterationResult result = checkConditionsForBalancing();
- if (result != null) {
- return result;
+ if (checkConditionsForBalancing() != null) {
Review comment:
I think the `maxSizeToMovePerIteration` and `maxDatanodesPercentageToInvolvePerIteration` are more like ITERATION_STOP_REASON other than ITERATION_RESULT.
Maybe we can add a new field noting the reason and mark the result as COMPLETE if encounter a valid STOP_REASON?
--
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