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_r288634531
##########
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:
> My problem with this solution from a design point is, that there is code
in the stream task that is talking about how the timer service wants to enqueue
things into the mailbox.
It's not "how the timer service wants". But it's rather that the
`StreamTask`'s use case, how to react to timer events. Optionally, this helper
class can be moved outside of `StreamTask`, but then it have to have reference
to checkpoint lock, `AsyncExceptionHandler` and `MailboxSender` from the
`StreamTask`.
> From a high-level view, do you agree that this code should not belong
there?
Imo, such code should not belong to timer service, but is fine to be in
`StreamTask`, as it's more coupled with the latter (`StreamTask`).
> Depending on what you think about this general concern, there is also a
solution that does all this inside the timer service, and the timer service
just knows the sender interface of the queue (and maybe the error handler). You
could almost move the code into the timer service, one the sender interface is
a member there.
Yes, this is basically to revert the change and additionally add
`MailboxSender` into the timer service.
It's similar to moving `TimerInvocationContext` into standalone class, and
then inlining it in the timer service, as the only implementation.
> Does that solve the problem of why you did not want to do this?
Personally, I'm against of what you propose. As I've mentioned before, it
would make (wrong) things more coupled.
----------------------------------------------------------------
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
