squah-confluent commented on code in PR #23913:
URL: https://github.com/apache/kafka/pull/23913#discussion_r4378971691
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorTimerImpl.java:
##########
@@ -166,6 +225,30 @@ public boolean isScheduled(String key) {
}
public void cancelAll() {
+ states.clear();
+ cancelTasks();
+ }
+
+ /**
+ * Rebuilds the scheduled tasks after the supplied snapshot registry has
been reverted.
+ * This must be called under the same external synchronization as the
registry rollback.
+ * It preserves the restored states and their original deadlines.
+ */
+ public void onRollback() {
+ cancelTasks();
+ // A timer implementation may execute an expired task immediately, so
iterate over
+ // a copy in case that task updates the states while we are
rescheduling them.
+ new HashMap<>(states).forEach(
+ (key, state) ->
+ scheduleTask(
+ key,
+ state,
+ Math.max(0, state.deadlineMs() - time.hiResClockMs())
+ )
+ );
+ }
Review Comment:
> 2. Timers scheduled by failure callbacks to restore the previous state
must survive that recovery.
I'm expecting these to be a manual implementation of rolling back timer
state. Hopefully they should all disappear at the end of this PR?
> 3. Changes such as extending a session timeout upon receiving a heartbeat
require a separate policy decision: should they also be reverted when an
unrelated write fails?
For simplicity I think unextending the session timeout is best. A consumer
would have to miss more than one heartbeat to be evicted from a group. We could
consider write failures to be rare enough that it's not worth the extra design
effort. @dajac any thoughts?
--
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]
