NealSun96 commented on a change in pull request #1326:
URL: https://github.com/apache/helix/pull/1326#discussion_r483253349
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/RebalanceScheduler.java
##########
@@ -80,6 +80,7 @@ public void scheduleRebalance(HelixManager manager, String
resource, long startT
long delay = startTime - System.currentTimeMillis();
if (delay < 0) {
LOG.debug(String.format("Delay time is %s, will not be scheduled",
delay));
+ return;
Review comment:
The fix was done based on the expectation of this function, as seen in
the comment: add a future rebalance task. In my opinion this function's duty is
for future rebalance scheduling, therefore a start time in the past should be
rejected as shown in the log.
If a user wants to schedule an immediate rebalance, we have
`scheduleOnDemandRebalance`. If a user is calling this function incorrectly
(passing in a past time), then the problem should show up during tests (by not
triggering rebalance) instead of hiding it and let it slip to production; if a
user is using this function correctly and wants to schedule a rebalance in the
future at time=x, but somehow this function is delayed and is processed at
time=x+1, then should that rebalance still happen? I don't think we should
assume it's okay, therefore it should be rejected.
@jiajunwang Functionally I understand your point; code-quality-wise I think
this function should do what it advertises. If there's a problem it should be
caught by tests and the negative delay should be handled during development.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
