vamossagar12 commented on PR #12826: URL: https://github.com/apache/kafka/pull/12826#issuecomment-1306629502
> @vamossagar12 Can you expand on the importance of this change? > > Is the single `long` with `0` as a sentinel any different in readability than a `Timer` object with `null` as a sentinel value? > > And are the improved monotonicity guarantees important to this use-case? As far as I can tell the rebalance delay value itself is purely advisory, and all that matters functionally is whether the timer is expired or not. > > Also: GitHub shows the removed lines in the diff, so it is more unclear to comment out the previous code than it is to remove it entirely before committing. You can always revert individual blocks locally with `git checkout HEAD^ -p`. > > Thanks! @gharris1727 thanks for your review. As I said it's not a change which is absolutely mandatory to make because all the logic baked into the Assignor class has been working for a while now. Regarding this: `Is the single `long` with `0` as a sentinel any different in readability than a `Timer` object with `null` as a sentinel value` yeah that null check is not very pretty but it's not the only thing right? The timer class already handles the entire lifecycle of a window and exposes methods. So, it becomes easier to reason about it like if the timer is expired or not using `timer.isExpired()` vis-a-vis the current way i.e `now >= scheduledRebalanceDelay`. Again, this is my point of view and with time one gets used to either way of coding. Regarding monotonicity, I don't think it's an absolute deterrent for the algorithm per se, but the algorithm does rely on system time when computing `now`. Not sure how it can break anything on the algorithm though. Lastly, about the commented bit, yeah I dont think it's needed. Sorry about the noise there. -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
