dajac commented on code in PR #23913:
URL: https://github.com/apache/kafka/pull/23913#discussion_r4349756755
##########
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:
@chickenchickenlove Thanks for the patch! Does it mean that we must cancel
and re-schedule all the timers on rollbacks? This seems pretty expensive, isn't
it?
I'd like to suggest an alternative design. One approach would be to only
keep the timers made by the operations of the current batch as pending until
the batch is successfully written. When it is, the pending timers are moved to
the "committed" space. If the write fails, they are dropped. We don't really
need to full-fleshed rollback to any previous committed offset in the timer
case.
It basically means that a timer is not really overridden until committed. In
this case, we must define what to do if the previous one fires before the new
one is committed. This may also be true in your design.
I have not tested it. It is just an idea. 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]
