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 Tue, 28 May 2019 18:58:43 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_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:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message