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-14750][configuration] Migrate duration and memory size ConfigOptions in RestartStrategyOptions #10216
Conversation
…ptions in RestartStrategyOptions
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 304d091 (Wed Dec 04 15:23:33 UTC 2019) 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:
|
@@ -61,7 +61,7 @@ public void createRestartStrategyFactory_fixedDelayRestartStrategyConfigured_ret | |||
final Configuration configuration = new Configuration(); | |||
configuration.setString(RestartStrategyOptions.RESTART_STRATEGY, "fixed-delay"); | |||
configuration.setInteger(RestartStrategyOptions.RESTART_STRATEGY_FIXED_DELAY_ATTEMPTS, attempts); | |||
configuration.setString(RestartStrategyOptions.RESTART_STRATEGY_FIXED_DELAY_DELAY, delayBetweenRestartAttempts.getSeconds() + "s"); | |||
configuration.setString(RestartStrategyOptions.RESTART_STRATEGY_FIXED_DELAY_DELAY.key(), delayBetweenRestartAttempts.getSeconds() + "s"); |
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.
Wondering whether we should just use a Duration
here.
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.
We can yes. It will no longer test the string parsing, but it does not have to. The parsing is covered in the Configuration
tests.
@@ -162,6 +162,14 @@ public boolean equals(Object obj) { | |||
} | |||
} | |||
|
|||
@Override | |||
public String toString() { | |||
return "FixedDelayRestartStrategyConfiguration{" + |
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.
Why is this being added? (and why not also for the other configurations?)
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.
You are right. I should've added it to all classes. I added it for debugging.
I added a common implementation to the base class that reuses the description.
283e9a5
to
304d091
Compare
<td>Delay between two consecutive restart attempts if <span markdown="span">`restart-strategy`</span> has been set to <span markdown="span">`failure-rate`</span>. It can be specified using notation: "1 min", "20 s"</td> | ||
</tr> | ||
<tr> | ||
<td><h5>restart-strategy.failure-rate.failure-rate-interval</h5></td> | ||
<td style="word-wrap: break-word;">"1 min"</td> | ||
<td>String</td> | ||
<td style="word-wrap: break-word;">60000ms</td> |
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.
as a future note for the generator, if the value in milliseconds is a lot larger we may want to display this in seconds/minutes instead.
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.
+1
What is the purpose of the change
Use typed
ConfigOptions
for RestartStrategyOptions. The options can be changed directly without deprecating first, because they haven't been released yet. Those options were introduced in 1.10.Verifying this change
This change is already covered by existing tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation