qinf commented on code in PR #23247:
URL: https://github.com/apache/flink/pull/23247#discussion_r1417423381
##########
flink-runtime/src/test/java/org/apache/flink/runtime/executiongraph/failover/ExponentialDelayRestartBackoffTimeStrategyTest.java:
##########
@@ -36,35 +36,37 @@ class ExponentialDelayRestartBackoffTimeStrategyTest {
private final Exception failure = new Exception();
@Test
- void testAlwaysRestart() throws Exception {
+ void testMaxAttempts() {
+ int maxAttempts = 13;
final ExponentialDelayRestartBackoffTimeStrategy restartStrategy =
new ExponentialDelayRestartBackoffTimeStrategy(
- new ManualClock(), 1L, 3L, 2.0, 4L, 0.25);
+ new ManualClock(), 1L, 3L, 2.0, 4L, 0.25, maxAttempts);
Review Comment:
The default value of `ConfigOption`
`restart-strategy.exponential-delay.backoff-multiplier` has been set to `1.2`.
Is it necessary to update backoffMultiplier to `1.2`?
##########
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/failover/RestartBackoffTimeStrategyFactoryLoader.java:
##########
@@ -116,7 +116,9 @@ private static Optional<RestartBackoffTimeStrategy.Factory>
getJobRestartStrateg
exponentialDelayConfig.getMaxBackoff().toMilliseconds(),
exponentialDelayConfig.getBackoffMultiplier(),
exponentialDelayConfig.getResetBackoffThreshold().toMilliseconds(),
- exponentialDelayConfig.getJitterFactor()));
+ exponentialDelayConfig.getJitterFactor(),
+
RestartStrategyOptions.RESTART_STRATEGY_EXPONENTIAL_DELAY_ATTEMPTS
Review Comment:
The `ExponentialDelayRestartStrategyConfiguration` contains other parameters
needed to create `ExponentialDelayRestartBackoffTimeStrategyFactory`. Is it
necessary to add a parameter `attemptsBeforeResetBackoff` to
`ExponentialDelayRestartStrategyConfiguration`?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/executiongraph/failover/ExponentialDelayRestartBackoffTimeStrategyTest.java:
##########
@@ -73,14 +75,14 @@ void testMaxBackoff() throws Exception {
}
@Test
- void testResetBackoff() throws Exception {
+ void testResetBackoff() {
final long initialBackoffMS = 1L;
final long resetBackoffThresholdMS = 8L;
final ManualClock clock = new ManualClock();
final ExponentialDelayRestartBackoffTimeStrategy restartStrategy =
new ExponentialDelayRestartBackoffTimeStrategy(
- clock, initialBackoffMS, 5L, 2.0,
resetBackoffThresholdMS, 0.25);
+ clock, initialBackoffMS, 5L, 2.0,
resetBackoffThresholdMS, 0.25, 100);
Review Comment:
Could you help explain why set the `attemptsBeforeResetBackoff` to 100 here?
--
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.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
