1u0 commented on a change in pull request #8523: [FLINK-12481][runtime] Invoke
timer callback in task thread (via mailbox)
URL: https://github.com/apache/flink/pull/8523#discussion_r288254960
##########
File path:
flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/tasks/StreamTask.java
##########
@@ -1358,4 +1358,19 @@ public void actionsUnavailable() throws
InterruptedException {
mailbox.putMail(actionUnavailableLetter);
}
}
+
+ private class TimerInvocationContext implements
SystemProcessingTimeService.ScheduledCallbackExecutionContext {
Review comment:
I don't think that the mailbox is exposed to the
`SystemProcessingTimeService` at all here. On the contrary.
The `TimerInvocationContext` is in fact a "mailbox sender" (implementation
detail, that doesn't matter for the timer service). I'm not using the real
`MailboxSender` interface, because it would involve somehow adapting
`ProcessingTimeCallback` interface to `Runnable`. Or changing all implementers
of `ProcessingTimeCallback` to new api - this may be quite involving and may
conflict with others' changes.
Imo, things like `SystemProcessingTimeService` can be neutral about use
cases. The fact that it already had checkpoint lock and error handler callback,
I find as incidental, rather by design.
Having additional transformation of `ProcessingTimeCallback` in the time
service, into a mailbox letter (`Runnable`), would make things even more
coupled.
An alternative could be, that there are no newly introduced
`SystemProcessingTimeService.ScheduledCallbackExecutionContext`, but keeping
`SystemProcessingTimeService` implementation clean (not coupled with
`StreamTask`).
Add a proxy `ProcessingTimeService` implementation that would be exposed for
others to register timers, but upon registration, the proxy would register it's
own callback, which when fired, places the original `ProcessingTimeCallback` as
a letter into the mailbox.
----------------------------------------------------------------
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:
us...@infra.apache.org
With regards,
Apache Git Services
