Copilot commented on code in PR #11000:
URL: https://github.com/apache/ozone/pull/11000#discussion_r3771239892
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerConfiguration.java:
##########
@@ -112,9 +112,13 @@ public final class ContainerBalancerConfiguration {
private long moveReplicationTimeout = Duration.ofMinutes(50).toMillis();
@Config(key = "hdds.container.balancer.balancing.iteration.interval", type =
ConfigType.TIME,
- defaultValue = "70m", tags = {ConfigTag.BALANCER}, description =
- "The interval period between each iteration of Container Balancer.")
- private long balancingInterval = Duration.ofMinutes(70).toMillis();
+ defaultValue = "3m", tags = {ConfigTag.BALANCER}, description =
+ "The interval period to wait between each iteration of Container " +
+ "Balancer. Container Balancer runs iterations continuously and only
" +
+ "waits this short period so that SCM receives refreshed datanode " +
Review Comment:
The docs still list `hdds.container.balancer.balancing.iteration.interval`
default as `70m` (see `hadoop-hdds/docs/content/feature/ContainerBalancer.md`
table). With the default changed to `3m` here, the documentation should be
updated to avoid misleading operators.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java:
##########
@@ -546,6 +554,19 @@ private void
validateConfiguration(ContainerBalancerConfiguration conf)
validateEligibleDatanodePool(conf);
}
+ /**
+ * A balancing interval that is not greater than the node report interval
means
+ * SCM may not receive refreshed datanode usage info before the next
iteration
+ * starts. Datanode usage is tracked incrementally in memory and reported
every
+ * node report interval, so that interval (not the hourly du refresh period)
is
+ * the relevant freshness bound between iterations.
+ */
+ @VisibleForTesting
+ static boolean isBalancingIntervalBelowNodeReportInterval(
+ long balancingIntervalMs, long nodeReportIntervalMs) {
+ return balancingIntervalMs <= nodeReportIntervalMs;
+ }
Review Comment:
`isBalancingIntervalBelowNodeReportInterval` returns `true` when the
interval is *equal* to the node report interval (`<=`). The name reads like a
strict "below" check; consider renaming to reflect the inclusive comparison (or
change the comparison to `<` if equality should be allowed).
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java:
##########
@@ -459,6 +459,33 @@ public void testRejectInvalidStartupConfiguration() throws
Exception {
assertSame(ContainerBalancerTask.Status.STOPPED,
containerBalancer.getBalancerStatus());
}
+ /**
+ * Container Balancer runs iterations continuously with only a short wait in
+ * between, so the default balancing interval is 3 minutes.
+ */
+ @Test
+ public void testDefaultBalancingIntervalIsThreeMinutes() {
+ ContainerBalancerConfiguration defaultConfig =
+ new
OzoneConfiguration().getObject(ContainerBalancerConfiguration.class);
+ assertEquals(3, defaultConfig.getBalancingInterval().toMinutes());
+ }
+
+ /**
+ * The freshness bound for the balancing interval is the node report interval
+ * (not the hourly du refresh period). The
+ * default 3m interval is above a typical 1m node report interval.
+ */
Review Comment:
Javadoc wraps a sentence mid-phrase ("...refresh period). The"), which reads
awkwardly. Consider reflowing the comment to avoid splitting the sentence.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
