1u0 commented on a change in pull request #9772: [FLINK-14199] Only use
dedicated/named classes for mailbox letters.
URL: https://github.com/apache/flink/pull/9772#discussion_r333381245
##########
File path:
flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/TaskMailboxImpl.java
##########
@@ -111,11 +111,11 @@ public boolean hasMail() {
//------------------------------------------------------------------------------------------------------------------
@Override
- public void putMail(@Nonnull Runnable letter, int priority) throws
MailboxStateException {
+ public void putMail(@Nonnull Runnable letter, int priority, Object
description) throws MailboxStateException {
final ReentrantLock lock = this.lock;
lock.lock();
try {
- putTailInternal(new Mail(letter, priority));
+ putTailInternal(new Mail(letter, priority,
description));
Review comment:
With current changes in this PR, the `TaskMailbox` api is not "symmetric":
the take mail methods return values of `Mail`, but put mail methods accept
`Runnable`.
What do you think about making the api more consistent, so user of the
interface works with the same letter entity class. You can do it by making put
mail methods to accept `Mail` object directly.
If you continue thinking in this direction: once you have a dedicated class
for mail entity (instead of more generic `Runnable`), why not to just directly
inherent from that class for concrete letters implementations?
This also brings us back to the approach to just inherent from the
`Runnable` and add a descriptive `toString()` override - that would make the
change less intrusive and smaller, imo.
----------------------------------------------------------------
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
