[FLINK-13884] Set default delay for restart strategies to 1s #9637

tillrohrmann · 2019-09-06T09:59:20Z

What is the purpose of the change

This PR is based on #9562.

This commit sets the default delay for all restart strategies to 1s in order
to prevent restart storms from happening.

cc @zentol @zhuzhurk

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

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, Yarn/Mesos, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (docs)

flinkbot · 2019-09-06T10:01:42Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 64fbd2a (Wed Oct 16 08:15:29 UTC 2019)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Details

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 commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2019-09-06T10:06:03Z

CI report:

a4d0425 : SUCCESS Build
6e6addf : FAILURE Build
9b959c0 : FAILURE Build
64fbd2a : SUCCESS Build

zentol · 2019-09-06T10:19:45Z

...nk/runtime/executiongraph/restart/NoOrFixedIfCheckpointingEnabledRestartStrategyFactory.java

 */
 public class NoOrFixedIfCheckpointingEnabledRestartStrategyFactory extends RestartStrategyFactory {
-	private static final long DEFAULT_RESTART_DELAY = 0;
+	private static final long DEFAULT_RESTART_DELAY = Duration.ofSeconds(1L).toMillis();


can we make this dependent on the default delay for the FixedDelayStrategy?

We would need to parse the string into a long. I would like to avoid to do this in a static block because if this fails, then the JVM process will fail to start.

But we could parse it when creating the NoOrFixedDelayRestartStrategy in the create method.

Did you ultimately decide against it? @tillrohrmann

Yes, we can change it once we have replaced Scala Duration parsing logic. I didn't want to further spread the usage of the Scala classes in the flink-runtime module.

zentol · 2019-09-06T10:19:48Z

...ts/src/test/java/org/apache/flink/test/recovery/SimpleRecoveryFailureRateStrategyITBase.java

 		config.setInteger(RestartStrategyOptions.RESTART_STRATEGY_FAILURE_RATE_MAX_FAILURES_PER_INTERVAL, 1);
 		config.setString(RestartStrategyOptions.RESTART_STRATEGY_FAILURE_RATE_FAILURE_RATE_INTERVAL, "1 second");
-		config.setString(RestartStrategyOptions.RESTART_STRATEGY_FAILURE_RATE_DELAY, "100 ms");
+		config.setString(RestartStrategyOptions.RESTART_STRATEGY_FAILURE_RATE_DELAY, "0 s");


why are we reducing the delay here? This doesn't seem strictly necessary.

This is true. It speeds up the test though. I'll add it as a separate hotfix commit.

tillrohrmann · 2019-09-06T12:31:15Z

I've updated this PR to update the SimpleRecoveryFailureRateStrategyITBase in a separate commit and to use the TimeUtils#parseDuration in order to parse the delay strings. That way it was possible to parse the default value of RestartStrategyOptions#RESTART_STRATEGY_FIXED_DELAY_DELAY using the same utility when creating the FixedDelayRestartStrategy in the NoOrFixedDelayRestartStrategyFactory.

zhuzhurk

It's good news that the Flink framework now can get rid of scala Duration.
Maybe we should open a JIRA for duration parsing refactoring?
This seems not to a very small change and it's better other developers can be aware of this change.

I also find some previous parsing logics are not refactored( by search scala.concurrent.duration.Duration in java code), including
{MesosEntrypointUtils, TaskManagerConfiguration, TableConfigUtils}.

(ProcessShutDownThread does not parse duration strings but it is referencing the scala Duration. Since it is not used anymore, I opened a issue FLINK-14000 to remove it.)

zhuzhurk · 2019-09-07T06:29:24Z

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

 				.text(
 					"Time interval for measuring failure rate if %s has been set to %s. " +
-						"It can be specified using Scala's %s notation: \"1 min\", \"20 s\"",
+						"It can be specified using the notation: \"1 min\", \"20 s\", \"100 ms\"",


With this change fewer time units are supported.
Previously supported time units are
{
DAYS -> "d day",
HOURS -> "h hour",
MINUTES -> "min minute",
SECONDS -> "s sec second",
MILLISECONDS -> "ms milli millisecond",
MICROSECONDS -> "µs micro microsecond",
NANOSECONDS -> "ns nano nanosecond"
}
Current TimeUtils only supports "h", "min", "s" and "ms".

To avoid break existing job, we may need to support those time units supported by scala FiniteDuration.
And maybe we also need a doc to specify supported time units and link this description to it.

That's a valid point. Though, for the affected durations in this commits it probably does not make sense to specify these in days, microseconds, or nanoseconds. I would say that the changes in this commit are out of scope for a hotfix commit. Strictly speaking the changes in this commit even break backwards compatibility.

Edit: Now I understand the full extent of the issue that @zhuzhurk raised. There are many more notations that we do not support anymore (not only units). Scala's Duration also supported specifications such as "5 minutes", which TimeUtils does not.

Yes, I'll revert these commits. Somehow the urge to get rid of Scala dependencies just overcame me....

zhuzhurk · 2019-09-07T06:31:00Z

...rc/main/java/org/apache/flink/runtime/executiongraph/restart/FailureRateRestartStrategy.java

 	public static FailureRateRestartStrategyFactory createFactory(Configuration configuration) throws Exception {
 		int maxFailuresPerInterval = configuration.getInteger(RestartStrategyOptions.RESTART_STRATEGY_FAILURE_RATE_MAX_FAILURES_PER_INTERVAL);
-		String failuresIntervalString = configuration.getString(RestartStrategyOptions.RESTART_STRATEGY_FAILURE_RATE_FAILURE_RATE_INTERVAL);
-		String timeoutString = configuration.getString(AkkaOptions.WATCH_HEARTBEAT_INTERVAL);


I think it's not proper to change the default value of restart delay in the "Factor duration parsing..." commit.

Valid point. What is the justification for not falling back to WATCH_HEARTBEAT_INTERVAL anymore?

This change should not be part of the parsing commit. I'll change it. WATCH_HEARTBEAT_INTERVAL is no longer used and will be deprecated in #9553. Moreover, I think it is better to separate configurations and try to make things as explicit as possible without much magic happening underneath.

GJL · 2019-09-09T14:39:44Z

All Travis stages are currently failing. Example:

StandaloneResourceManagerFactoryTest.createResourceManager_WithLessMemoryThanContainerizedHeapCutoffMin_ShouldSucceed

org.apache.flink.configuration.IllegalConfigurationException: Invalid duration configuration value for resourcemanager.job.timeout: 5 minutes. Value must be a valid duration (such as '100 ms' or '10 s').

	at org.apache.flink.configuration.ConfigurationUtils.parseDuration(ConfigurationUtils.java:162)
	at org.apache.flink.runtime.resourcemanager.ResourceManagerRuntimeServicesConfiguration.fromConfiguration(ResourceManagerRuntimeServicesConfiguration.java:55)
	at org.apache.flink.runtime.resourcemanager.StandaloneResourceManagerFactory.createResourceManager(StandaloneResourceManagerFactory.java:53)
	at org.apache.flink.runtime.resourcemanager.StandaloneResourceManagerFactoryTest.createResourceManager_WithLessMemoryThanContainerizedHeapCutoffMin_ShouldSucceed(StandaloneResourceManagerFactoryTest.java:54)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:55)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
Caused by: java.lang.IllegalArgumentException: Time interval unit 'minutes' does not match any of the recognized units: ms | s | min | h
	at org.apache.flink.util.TimeUtils.parseDuration(TimeUtils.java:81)
	at org.apache.flink.configuration.ConfigurationUtils.parseDuration(ConfigurationUtils.java:159)
	... 27 more

tillrohrmann · 2019-09-09T16:38:27Z

@zhuzhurk and @GJL, I've removed the commit with the Duration parsing and force pushed an updated version. Please take a look.

zhuzhurk · 2019-09-10T06:10:51Z

The build breaks due to the default values of for "restart-strategy.failure-rate.delay" and "restart-strategy.fixed-delay.delay" are not updated in generated docs.

I think we can merge this PR once the docs are regenerated.

…e to 0 s This speeds up the SimpleRecoveryFailureRateStrategyITBase.

This commit sets the default delay for all restart strategies to 1s in order to prevent restart storms from happening.

tillrohrmann · 2019-09-10T07:10:55Z

Thanks for the pointer @zhuzhurk. I've regenerated the documentation and pushed an update.

tillrohrmann · 2019-09-10T11:47:47Z

Thanks for the review everyone. Merging this PR now.

rmetzger added the review=description? label Sep 6, 2019

tillrohrmann mentioned this pull request Sep 6, 2019

[FLINK-13883] Deprecate unused AkkaOptions related to Akka's death watch #9638

Closed

zentol self-assigned this Sep 6, 2019

zentol reviewed Sep 6, 2019

View reviewed changes

rmetzger added the component=Runtime/Coordination label Sep 6, 2019

tillrohrmann force-pushed the FLINK-13884 branch from a4d0425 to 6e6addf Compare September 6, 2019 12:28

zhuzhurk reviewed Sep 7, 2019

View reviewed changes

GJL assigned GJL and unassigned zentol Sep 9, 2019

tillrohrmann force-pushed the FLINK-13884 branch from 6e6addf to 9b959c0 Compare September 9, 2019 16:36

tillrohrmann added 3 commits September 10, 2019 09:10

[hotfix] Fix checkstyle violations in FailureRateRestartStrategy

677094a

[hotfix] Set restart delay for SimpleRecoveryFailureRateStrategyITBas…

1443f7e

…e to 0 s This speeds up the SimpleRecoveryFailureRateStrategyITBase.

[FLINK-13884] Set default delay for restart strategies to 1s

64fbd2a

This commit sets the default delay for all restart strategies to 1s in order to prevent restart storms from happening.

tillrohrmann force-pushed the FLINK-13884 branch from 9b959c0 to 64fbd2a Compare September 10, 2019 07:10

GJL approved these changes Sep 10, 2019

View reviewed changes

tillrohrmann closed this in c3726cd Sep 10, 2019

tillrohrmann deleted the FLINK-13884 branch September 10, 2019 12:13

[FLINK-13884] Set default delay for restart strategies to 1s #9637

[FLINK-13884] Set default delay for restart strategies to 1s #9637

Uh oh!

Conversation

tillrohrmann commented Sep 6, 2019

What is the purpose of the change

Verifying this change

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

Documentation

Uh oh!

flinkbot commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automated Checks

Review Progress

Uh oh!

flinkbot commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tillrohrmann commented Sep 6, 2019

Uh oh!

zhuzhurk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GJL Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GJL commented Sep 9, 2019

Uh oh!

tillrohrmann commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhuzhurk commented Sep 10, 2019

Uh oh!

tillrohrmann commented Sep 10, 2019

Uh oh!

tillrohrmann commented Sep 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

flinkbot commented Sep 6, 2019 •

edited

Loading

flinkbot commented Sep 6, 2019 •

edited

Loading

zhuzhurk left a comment •

edited

Loading

GJL Sep 9, 2019 •

edited

Loading

tillrohrmann commented Sep 9, 2019 •

edited

Loading