jiajunwang commented on a change in pull request #1326:
URL: https://github.com/apache/helix/pull/1326#discussion_r483345696
##########
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:
1. If this change is not absolutely necessary for this PR, let's split.
2. Does it really cause any trouble for us? We should do the right thing,
not what has been written down as a comment.
The potential problem I see here is that, as you mentioned, the possible gap
between the timestamp generated and the time when this method starts
processing. The delayed rebalancer has this potential issue if you return
prematurely. So if you want to change it, then you need to "fix" the delayed
rebalance. But back to the first point, I don't think it is a problem and the
comment is the only thing we shall fix.
Anyway, please convince me with a scenario that without return, our logic
will fail. Otherwise, #1325 does not look like an issue to me.
----------------------------------------------------------------
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]
