pnowojski commented on a change in pull request #9383: [FLINK-13248] [runtime]
Adding processing of downstream messages in AsyncWaitOperator's wait loops
URL: https://github.com/apache/flink/pull/9383#discussion_r314330128
##########
File path:
flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/TaskMailboxImpl.java
##########
@@ -84,10 +89,14 @@ public boolean hasMail() {
@Override
public Optional<Runnable> tryTakeMail() throws MailboxStateException {
+ return downstreamMailbox.tryTakeMail();
+ }
+
+ private Optional<Runnable> tryTakeDownstreamMail(int operatorIndex)
throws MailboxStateException {
Review comment:
I was a bit confused about the class interactions here and the call chain is
a bit complicated (`TaskMailboxImpl #tryTakeMail` -> goes to downstreamMailbox
`downstreamMailbox.tryTakeMail()` -> comes back here via
`TaskMailboxImpl#tryTakeDownstreamMail`). Would it be simpler if dropped the
requirement of `TaskMailbox` and `Mailbox` having the same interface?
On the most upper level I would actually prefer having to explicitly pass
`operatorIndex` when calling `putMail`, to make this more explicit with less
magic. It would be more readable for me to have this lower level api explicit,
instead of figuring out what are the `putMail(mail)` or `tryTakeMail()` doing.
`putMail(mail, priority)` or `tryTakeMail(priority)` is more self explanatory
interface.
----------------------------------------------------------------
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
