[FLINK-32895][Scheduler] Introduce the max attempts for Exponential Delay Restart Strategy #23247

1996fanrui · 2023-08-21T07:15:25Z

What is the purpose of the change

See FLIP-364: Improve the exponential-delay restart-strategy . This PR includes the subtask2 of FLIP-364.

Brief change log

[FLINK-32895][Scheduler][hotfix] Generate latest strategy string for ExponentialDelayRestartBackoffTimeStrategy due to some information will be updated, such as: currentBackoffMS and lastFailureTimestamp
[FLINK-32895][Scheduler] Introduce the max attempts for Exponential Delay Restart Strategy
- Add the option: restart-strategy.exponential-delay.attempts-before-reset-backoff
[FLINK-32895][Scheduler][refactor] Remove some unused throws

Verifying this change

Improve some tests in ExponentialDelayRestartBackoffTimeStrategyTest.
Change the testAlwaysRestart to testMaxAttempts in ExponentialDelayRestartBackoffTimeStrategyTest.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): yes
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? yes
If yes, how is the feature documented? Java doc and config doc are updated, and updated a little official web doc related to restart-strategy.exponential-delay.attempts-before-reset-backoff.

flinkbot · 2023-08-21T07:19:20Z

CI report:

08efd3f Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

1996fanrui

Hey @RocMarshal @qinf , would you mind helping take a look this PR in your free time? thanks~

qinf

@1996fanrui Thank you for your PR. I have a few comments, please take a look.

qinf · 2023-12-06T14:33:46Z

...he/flink/runtime/executiongraph/failover/ExponentialDelayRestartBackoffTimeStrategyTest.java

        final ExponentialDelayRestartBackoffTimeStrategy restartStrategy =
                new ExponentialDelayRestartBackoffTimeStrategy(
-                        new ManualClock(), 1L, 3L, 2.0, 4L, 0.25);
+                        new ManualClock(), 1L, 3L, 2.0, 4L, 0.25, maxAttempts);


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?

qinf · 2023-12-06T14:35:43Z

...he/flink/runtime/executiongraph/failover/ExponentialDelayRestartBackoffTimeStrategyTest.java

        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);


Could you help explain why set the attemptsBeforeResetBackoff to 100 here?

It doesn't have strong reason, I just set it to a big value to ensure this test doesn't reach this limitation.

Update attemptsBeforeResetBackoff to Integer.MAX_VALUE for all tests not related to maxAttempts.

Integer.MAX_VALUE makes it easier for other developers to know that attemptsBeforeResetBackoff does not take effect.

qinf · 2023-12-06T14:46:12Z

...rg/apache/flink/runtime/executiongraph/failover/RestartBackoffTimeStrategyFactoryLoader.java

+                            exponentialDelayConfig.getJitterFactor(),
+                            RestartStrategyOptions.RESTART_STRATEGY_EXPONENTIAL_DELAY_ATTEMPTS


The ExponentialDelayRestartStrategyConfiguration contains other parameters needed to create ExponentialDelayRestartBackoffTimeStrategyFactory. Is it necessary to add a parameter attemptsBeforeResetBackoff to ExponentialDelayRestartStrategyConfiguration?

I didn't do it because ExponentialDelayRestartStrategyConfiguration is deprecated since 1.19. The RestartStrategies has been deprecated, and ExponentialDelayRestartStrategyConfiguration is the inner class of it.

So the new option won't be supportted in the deprecated api.

1996fanrui

Hi @zhuzhurk , would you mind helping double check this PR in your free time? Thanks~

This PR is the subtask1 and subtask2 of improving FLIP-364: Exponential Delay Restart Strategy.

RocMarshal

Thanks @1996fanrui for the contribution~
LGTM +1.

RocMarshal · 2023-12-11T11:13:03Z

flink-core/src/main/java/org/apache/flink/configuration/RestartStrategyOptions.java

+                            Description.builder()
+                                    .text(
+                                            "The number of times that Flink retries the execution before the job is declared as failed "
+                                                    + "before reset the backoff to its initial value if %s has been set to %s.",


reset -> resetting

Thanks @RocMarshal for the review! Updated~

mxm · 2023-12-11T15:13:28Z

flink-core/src/main/java/org/apache/flink/configuration/RestartStrategyOptions.java

@@ -184,7 +184,7 @@ public class RestartStrategyOptions {
    public static final ConfigOption<Double> RESTART_STRATEGY_EXPONENTIAL_DELAY_BACKOFF_MULTIPLIER =
            ConfigOptions.key("restart-strategy.exponential-delay.backoff-multiplier")
                    .doubleType()
-                    .defaultValue(2.0)
+                    .defaultValue(1.2)


Could we set this to 1.5 to make it a bit less aggressive?

Thanks @mxm for the feedback!

1.5 is fine for me, and I'd like to cc @zhuzhurk who propose change these 2 default value as well!

Max and Mason have a little feedback after voting in the user mail list, and Max and I had a offline discussion yesterday. Max think the 1.2 is a little small or aggressive(delay time is too short). Here is the reason:

Every time the job restarts, it will make a bunch of calls to the Kubernetes API, e.g. read/write to config maps, create task managers.

When his producation had the default fixed-delay(1s) restart strategy turned on. A Kubernetes cluster became instable.

Following is the relationship between restart-attempts and retry-delay-time:

The delay-time will reach 1 min after 12 attempts when backoff-multiplier is 1.5

The delay-time will reach 1 min after 24 attempts when backoff-multiplier is 1.2

Hey @zhuzhurk , what do you think about setting1.5 as the default value? If you agree it, I will update the FLIP and feedback it to the user mail list, and go ahead this PR.

1.5 sounds good to me. Yet I think we need to send the updates to the FLIP discussion ML and open another vote for it, in case it turned out to be a surprise for those who have reviewed and voted on the FLIP.

Thanks @zhuzhurk for the quick feedback!

I have updated it in the dev&user mail list. Let us wait for more feedback for a while. Also, I pause FLINK-33736 first. Therefore, I will remove the first commit from this PR and continue working on the rest of the JIRA for FLIP-364.

[1] https://lists.apache.org/thread/6glz0d57r8gtpzq4f71vf9066c5x6nyw

The default value related commit is migrated to a new separate PR, we can follow it there, thanks~

Also, if this PR(FLINK-32895) no other comments within 72 hours, I will merge it, thanks.

Thanks for considering our suggestion!

…g for ExponentialDelayRestartBackoffTimeStrategy due to some information will be updated

zhuzhurk

Thanks for creating this PR! @1996fanrui
I just have one minor comment for it.

zhuzhurk · 2023-12-12T08:37:05Z

flink-core/src/main/java/org/apache/flink/configuration/RestartStrategyOptions.java

+                            Description.builder()
+                                    .text(
+                                            "The number of times that Flink retries the execution before the job is declared as failed "
+                                                    + "before resetting the backoff to its initial value if %s has been set to %s.",


Maybe separate the long statement to make it easier for reading? e.g.

The number of times that Flink retries the execution before failing the job if %s has been set to %s. The number will be reset once the backoff is reset to its initial value.

Thanks @zhuzhurk for the great suggestion! It's easier to understand~

I updated the original commit directly due to this change is easy enough, and the change can be viewed by this link[1].

[1] https://github.com/apache/flink/compare/f91f422f3350b622715d140a1255746aafb6a1e7..08efd3f4e062490ac2431408bcaf3d079cf1e6bb

…elay Restart Strategy

zhuzhurk

LGTM.

1996fanrui · 2023-12-15T04:49:20Z

Thanks everyone for the review, merging~

1996fanrui force-pushed the 32895/exponnential-restart-max-attemepts branch 7 times, most recently from 7272d54 to 92401d9 Compare December 4, 2023 09:12

1996fanrui marked this pull request as ready for review December 4, 2023 09:19

1996fanrui force-pushed the 32895/exponnential-restart-max-attemepts branch from 92401d9 to c0de3c8 Compare December 4, 2023 11:22

1996fanrui commented Dec 4, 2023

View reviewed changes

qinf reviewed Dec 6, 2023

View reviewed changes

1996fanrui force-pushed the 32895/exponnential-restart-max-attemepts branch from c0de3c8 to 5374606 Compare December 7, 2023 05:19

qinf approved these changes Dec 11, 2023

View reviewed changes

1996fanrui commented Dec 11, 2023

View reviewed changes

RocMarshal approved these changes Dec 11, 2023

View reviewed changes

mxm reviewed Dec 11, 2023

View reviewed changes

1996fanrui force-pushed the 32895/exponnential-restart-max-attemepts branch from d6bdb4a to 3189fc0 Compare December 12, 2023 03:41

[FLINK-32895][Scheduler][refactor] Generate the latest strategy strin…

e355adc

…g for ExponentialDelayRestartBackoffTimeStrategy due to some information will be updated

1996fanrui force-pushed the 32895/exponnential-restart-max-attemepts branch from 3189fc0 to f91f422 Compare December 12, 2023 07:30

zhuzhurk reviewed Dec 12, 2023

View reviewed changes

1996fanrui added 2 commits December 12, 2023 17:22

[FLINK-32895][Scheduler] Introduce the max attempts for Exponential D…

0d9bec7

…elay Restart Strategy

[FLINK-32895][Scheduler][refactor] Remove some unused throws

08efd3f

1996fanrui force-pushed the 32895/exponnential-restart-max-attemepts branch from f91f422 to 08efd3f Compare December 12, 2023 09:23

zhuzhurk approved these changes Dec 13, 2023

View reviewed changes

1996fanrui merged commit 3d4d396 into apache:master Dec 15, 2023

1996fanrui deleted the 32895/exponnential-restart-max-attemepts branch December 15, 2023 04:49

flinkbot added the component=Runtime/Coordination label Apr 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-32895][Scheduler] Introduce the max attempts for Exponential Delay Restart Strategy #23247

[FLINK-32895][Scheduler] Introduce the max attempts for Exponential Delay Restart Strategy #23247

1996fanrui commented Aug 21, 2023 •

edited

flinkbot commented Aug 21, 2023 •

edited

1996fanrui left a comment

qinf left a comment

qinf Dec 6, 2023

qinf Dec 6, 2023

1996fanrui Dec 6, 2023

1996fanrui Dec 7, 2023

qinf Dec 6, 2023

1996fanrui Dec 6, 2023

1996fanrui left a comment •

edited

RocMarshal left a comment

RocMarshal Dec 11, 2023

1996fanrui Dec 12, 2023

mxm Dec 11, 2023

1996fanrui Dec 12, 2023 •

edited

zhuzhurk Dec 12, 2023 •

edited

1996fanrui Dec 12, 2023

1996fanrui Dec 12, 2023 •

edited

mxm Dec 12, 2023

zhuzhurk left a comment

zhuzhurk Dec 12, 2023

1996fanrui Dec 12, 2023

zhuzhurk left a comment

1996fanrui commented Dec 15, 2023

		exponentialDelayConfig.getJitterFactor(),
		RestartStrategyOptions.RESTART_STRATEGY_EXPONENTIAL_DELAY_ATTEMPTS

[FLINK-32895][Scheduler] Introduce the max attempts for Exponential Delay Restart Strategy #23247

[FLINK-32895][Scheduler] Introduce the max attempts for Exponential Delay Restart Strategy #23247

Conversation

1996fanrui commented Aug 21, 2023 • edited

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Aug 21, 2023 • edited

CI report:

1996fanrui left a comment

Choose a reason for hiding this comment

qinf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1996fanrui left a comment • edited

Choose a reason for hiding this comment

RocMarshal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1996fanrui Dec 12, 2023 • edited

Choose a reason for hiding this comment

zhuzhurk Dec 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1996fanrui Dec 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuzhurk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuzhurk left a comment

Choose a reason for hiding this comment

1996fanrui commented Dec 15, 2023

1996fanrui commented Aug 21, 2023 •

edited

flinkbot commented Aug 21, 2023 •

edited

1996fanrui left a comment •

edited

1996fanrui Dec 12, 2023 •

edited

zhuzhurk Dec 12, 2023 •

edited

1996fanrui Dec 12, 2023 •

edited