pnowojski commented on a change in pull request #10345: [FLINK-12484][runtime]
synchronize all mailbox actions
URL: https://github.com/apache/flink/pull/10345#discussion_r352514354
##########
File path:
flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/Mail.java
##########
@@ -52,8 +54,10 @@ public int getPriority() {
return priority;
}
- public Runnable getRunnable() {
- return runnable;
+ public void tryCancel(boolean mayInterruptIfRunning) {
+ if (runnable instanceof Future) {
+ ((Future<?>) runnable).cancel(mayInterruptIfRunning);
+ }
Review comment:
I'm not sure if that's the right place for this check. `MailboxExecutor` is
actually creating `FutureTask` instances, which is also a kind of private
implementation detail that the Runnable can be a `Future`. Maybe we should move
`tryCancel` method to the `MailboxExecutor` somehow?
Something is not right here, but this also was not right before and I'm not
entirely sure how to fix it. However I'm not entirely sure if the fix that you
proposed is much better. Previously `Mail` was exposing its `Runnable`, which
wasn't super pretty, but also wasn't a very big deal, but with this commit and
without it, this `instanceof Future` check is leaking through the code :(
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
