siddhantsangwan commented on code in PR #8490:
URL: https://github.com/apache/ozone/pull/8490#discussion_r2120132853
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java:
##########
@@ -459,6 +461,22 @@ private void
validateConfiguration(ContainerBalancerConfiguration conf)
"hdds.container.balancer.move.replication.timeout should " +
"be less than hdds.container.balancer.move.timeout.");
}
+
+ // (move.timeout - move.replication.timeout -
event.timeout.datanode.offset)
+ // should be greater than or equal to 9 minutes
+ long datanodeOffset =
ozoneConfiguration.getTimeDuration("hdds.scm.replication.event.timeout.datanode.offset",
+ Duration.ofMinutes(6).toMillis(), TimeUnit.MILLISECONDS);
+ if ((conf.getMoveTimeout().toMillis() -
conf.getMoveReplicationTimeout().toMillis() - datanodeOffset)
+ < Duration.ofMinutes(9).toMillis()) {
+ String msg = String.format("(hdds.container.balancer.move.timeout (%sm)
- " +
+ "hdds.container.balancer.move.replication.timeout (%sm) - " +
+ "hdds.scm.replication.event.timeout.datanode.offset (%sm))
should be greater than or equal to 9 minutes.",
Review Comment:
Small suggestion, it's better to just the log the duration in milliseconds
so we know precisely how much it is when parsing the logs.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java:
##########
@@ -205,6 +205,21 @@ public void testValidationOfConfigurations() {
() -> containerBalancer.startBalancer(balancerConfiguration),
"hdds.container.balancer.move.replication.timeout should " +
"be less than hdds.container.balancer.move.timeout.");
+ assertSame(ContainerBalancerTask.Status.STOPPED,
containerBalancer.getBalancerStatus());
+
+ conf.setTimeDuration("hdds.container.balancer.move.timeout", 60,
+ TimeUnit.MINUTES);
+ conf.setTimeDuration(
+ "hdds.container.balancer.move.replication.timeout", 50,
+ TimeUnit.MINUTES);
+
+ balancerConfiguration =
+ conf.getObject(ContainerBalancerConfiguration.class);
+ assertThrowsExactly(
+ InvalidContainerBalancerConfigurationException.class,
+ () -> containerBalancer.startBalancer(balancerConfiguration),
+ "should be greater than or equal to 9 minutes.");
Review Comment:
Since we're adding this assertion to an existing test, let's also make sure
the message of the exception is what we expect it to be, "... should be greater
than ..."
--
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]
