-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-35528][task] Skip execution of interruptible mails when yielding #24904
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general, I have some minor comments about the implementation, PTAL.
flink-core/src/main/java/org/apache/flink/api/common/operators/MailboxExecutor.java
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/operators/MailboxExecutor.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/operators/MailOptionsImpl.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/operators/MailOptionsImpl.java
Outdated
Show resolved
Hide resolved
flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/Mail.java
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/operators/MailboxExecutor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Overall looks good. I left some comments below and under your previous conversations :D
flink-core/src/main/java/org/apache/flink/api/common/operators/MailboxExecutor.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/operators/MailboxExecutor.java
Outdated
Show resolved
Hide resolved
flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/tasks/mailbox/Mail.java
Outdated
Show resolved
Hide resolved
7a0735d
to
dd831d2
Compare
flink-core/src/main/java/org/apache/flink/api/common/operators/MailOptionsImpl.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/operators/MailboxExecutor.java
Show resolved
Hide resolved
e9b1560
to
a547a9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the PR, LGTM
(except for docs/layouts/shortcodes/generated/execution_checkpointing_configuration.html
)
docs/layouts/shortcodes/generated/execution_checkpointing_configuration.html
Outdated
Show resolved
Hide resolved
13db505
to
13448db
Compare
When operators are yielding, for example waiting for async state access to complete before a checkpoint, it would be beneficial to not execute interruptible mails. Otherwise continuation mail for firing timers would be continuously re-enqeueed. To achieve that MailboxExecutor must be aware which mails are interruptible. The easiest way to achieve this is to set MIN_PRIORITY for interruptible mails.
This PR depends on #24895 . Only the last commit should be reviewed here.
When operators are yielding, for example waiting for async state access to complete before a checkpoint, it would be beneficial to not execute interruptible mails. Otherwise continuation mail for firing timers would be continuously re-enqeueed. To achieve that MailboxExecutor must be aware which mails are interruptible.
The easiest way to achieve this is to set MIN_PRIORITY for interruptible mails.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation