You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/11/05 18:07:53 UTC
[GitHub] [kafka] vamossagar12 opened a new pull request, #12826: Using Timer class to track expiry in IncrementalCooperativeAssignor
vamossagar12 opened a new pull request, #12826:
URL: https://github.com/apache/kafka/pull/12826
Currently, the IncrementalCooperativeAssignor keeps track of time i.e scheduledRebalance and delay within the class itself. While that has worked successfully, sometimes it is not very intuitive when reading the code. For example condition like
`if (scheduledRebalance > 0 && now >= scheduledRebalance) {` is used to check if the scheduled rebalance delay is expired for example. Similarly, the IncrementalCooperativeAssignor also calculates delay/scheduledRebalanceDelay within the codebase. This has worked in general but there could be edge cases when the underlying clock is subject to time slips for example.
The kafka codebases already has a Timer class which can be used for tracking time and is used by other components in Kafka like Consumer, NetworkClient, AbstractCoordinator etc. It exposes methods like `isExpired`, `notExpired`, `remainingTimeMs`, `elapsedTimeMs`, `deadlineMs` and does the calculations internally. Moreover, `remainingTimeMs` maps directly to `delay` in the IncrementalCooperativeAssignor and `deadlineMs` to `scheduledRebalance`. One last nice thing about the Timer class is that it guarantees monotonic behaviour even if the underlying clock is not.
This draft PR is an attempt to use `Timer` constructs in `IncrementalCooperativeAssignor` .
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 closed pull request #12826: Using Timer class to track expiry in IncrementalCooperativeAssignor
Posted by GitBox <gi...@apache.org>.
vamossagar12 closed pull request #12826: Using Timer class to track expiry in IncrementalCooperativeAssignor
URL: https://github.com/apache/kafka/pull/12826
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #12826: Using Timer class to track expiry in IncrementalCooperativeAssignor
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] gharris1727 commented on a diff in pull request #12826: Using Timer class to track expiry in IncrementalCooperativeAssignor
Posted by GitBox <gi...@apache.org>.
gharris1727 commented on code in PR #12826:
URL: https://github.com/apache/kafka/pull/12826#discussion_r1015749550
##########
clients/src/main/java/org/apache/kafka/common/utils/Timer.java:
##########
@@ -172,6 +172,17 @@ public long currentTimeMs() {
return currentTimeMs;
}
+ /**
+ * Get the deadline time in milliseconds when this timer would expire. The deadlineMs time
+ * is generally updated through {@link #reset(long)} method or {@link #updateAndReset(long)}
+ * method call.
+ *
+ * @return The deadline time in milliseconds at which the timer would expire.
+ */
+ public long deadlineMs() {
+ return deadlineMs;
+ }
+
Review Comment:
This method seems to only be useful in the test assertions, and that doesn't seem like a good enough reason to expose this functionality in the common utils.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #12826: Using Timer class to track expiry in IncrementalCooperativeAssignor
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12826:
URL: https://github.com/apache/kafka/pull/12826#issuecomment-1315551866
hey @gharris1727 , did you get a chance to read my comments above? Do you think it's something worthy of perceiving further?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] gharris1727 commented on pull request #12826: Using Timer class to track expiry in IncrementalCooperativeAssignor
Posted by GitBox <gi...@apache.org>.
gharris1727 commented on PR #12826:
URL: https://github.com/apache/kafka/pull/12826#issuecomment-1317721387
@vamossagar12 Thanks for the further explanation.
As proposed, I don't think this refactor improves readability and maintainability more than it costs to implement, review, and merge.
I think there are other potential changes that deserve your valuable attention more, and that we can leave this `long` as-is for a little longer.
Thanks!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] gharris1727 commented on pull request #12826: Using Timer class to track expiry in IncrementalCooperativeAssignor
Posted by GitBox <gi...@apache.org>.
gharris1727 commented on PR #12826:
URL: https://github.com/apache/kafka/pull/12826#issuecomment-1306029257
@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!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on a diff in pull request #12826: Using Timer class to track expiry in IncrementalCooperativeAssignor
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12826:
URL: https://github.com/apache/kafka/pull/12826#discussion_r1016131415
##########
clients/src/main/java/org/apache/kafka/common/utils/Timer.java:
##########
@@ -172,6 +172,17 @@ public long currentTimeMs() {
return currentTimeMs;
}
+ /**
+ * Get the deadline time in milliseconds when this timer would expire. The deadlineMs time
+ * is generally updated through {@link #reset(long)} method or {@link #updateAndReset(long)}
+ * method call.
+ *
+ * @return The deadline time in milliseconds at which the timer would expire.
+ */
+ public long deadlineMs() {
+ return deadlineMs;
+ }
+
Review Comment:
hmm maybe it wasn't needed before? I see no difference in exposing this v/s exposing `currentTimeMs` for example. Let me know what you think about it?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #12826: Using Timer class to track expiry in IncrementalCooperativeAssignor
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12826:
URL: https://github.com/apache/kafka/pull/12826#issuecomment-1304603537
Hey @C0urante , I created this draft PR to use `Timer` constructs in `IncrementalCooperativeAssignor`. I have added the rationale as well. You could argue that why fix something which is not broken as well :) but I thought it makes sense to do this. Let me know what you think about it.
PS: I haven't added any new tests as with this change, the tests/end users shouldn't notice any difference. I ran all tests locally and they seemed to have passed which should be enough IMO.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #12826: Using Timer class to track expiry in IncrementalCooperativeAssignor
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12826:
URL: https://github.com/apache/kafka/pull/12826#issuecomment-1318417587
@gharris1727 Thanks for the confirmation! Let me close this PR.
BTW: `we can leave this long as-is for a little longer.` this was nice :)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org