-
Notifications
You must be signed in to change notification settings - Fork 13k
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-26331] Make cleanup retry strategy configurable similar to how the task restart is configurable #18913
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit a2dec76 (Thu Feb 24 18:44:30 UTC 2022) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
8874d2b
to
0ba635c
Compare
c40adf1
to
53171bb
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.
A few small documentation suggestions, and just a comment so I better understand what the enum factory pattern is for - but otherwise lgtm!
flink-core/src/main/java/org/apache/flink/configuration/CleanupOptions.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/configuration/CleanupOptions.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/configuration/CleanupOptions.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/org/apache/flink/runtime/dispatcher/cleanup/CleanupRetryStrategyFactory.java
Show resolved
Hide resolved
…configuration pattern for task restarts The issue is that repeatable cleanups leverage the RetryStrategy. In contrast, task restarts use RestartBackoffTimeStrategy. We might want to align these two approaches and provide a generic interface to allow retries. FLINK-26359 was created to cover this.
Thanks @autophagy for your review. I applied your comments, squashed the commits and rebased the branch. I will merge it after the final CI run succeeds. |
Test failure unrelated to changes: FLINK-26387 I'm going to merge the PR |
What is the purpose of the change
The goal is to align the user experience for retry functionality. I created a follow-up ticket FLINK-26359 to cover the actual alignment in the code.
Brief change log
CleanupOptions
similar toRestartOptions
to give a consistent user experience.CleanupRetryStrategyFactory
covering the configuration extraction to create the rightRetryStrategy
instanceVerifying this change
CleanupRetryStrategyFactoryTest
is added to cover all different optionsDoes this pull request potentially affect one of the following parts:
@Public(Evolving)
: yesDocumentation