flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [flink] 1u0 commented on a change in pull request #8523: [FLINK-12481][runtime] Invoke timer callback in task thread (via mailbox)
Date Wed, 29 May 2019 15:40:22 GMT
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:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message