[FLINK-13581][coordination][tests] Harden BatchFineGrainedRecoveryITCase #9374

azagrebin · 2019-08-06T15:01:54Z

What is the purpose of the change

If counting of mapper restarts in BatchFineGrainedRecoveryITCase is based on the open method of user function, the fact of the restart depends on internal implementation of the local Task and whether the open method is eventually called.

If execution attempt numbers are used instead, the test behaviour is more stable because it depends only on coordination. The execution attempt numbers can be queried from the REST client of the testing mini cluster.

Brief change log

Introduce MiniClusterClient for BatchFineGrainedRecoveryITCase to query task attempts
Use mapper task attempt numbers instead of user function open method call counters

Verifying this change

Run BatchFineGrainedRecoveryITCase.

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): (no)
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? (not applicable)

flinkbot · 2019-08-06T15:04:24Z

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 03733d7 (Thu Aug 08 12:15:42 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-08-06T15:12:55Z

CI report:

1b9f6d0 : CANCELED Build
a706bbd : SUCCESS Build
bf7f688 : FAILURE Build
69613de : FAILURE Build
03733d7 : FAILURE Build

tillrohrmann

Thanks for opening this PR @azagrebin. I had some comments mainly around the usage of RestClusterClient vs. RestClient.

I know that this test tests some complex behaviour, but I have to admit that it's not easy to actually follow what's going on. Hence I'm a bit concerned about its maintainability in the long run. Nothing we have to address right away but maybe we can rework this test to be a bit simpler and easier to understand.

tillrohrmann · 2019-08-07T10:09:42Z

flink-core/src/main/java/org/apache/flink/util/ExceptionUtils.java

+			rethrow(t);
+			throw new RuntimeException("Unreachable", t); // to prevent returning null and IDE null analysis
+		}
+	}


Should this maybe go into the SupplierWithException interface as SupplierWithException.join (similarly to CompletableFuture.join)? Adding it as a default implementation should not break the interface.

tillrohrmann · 2019-08-07T10:10:48Z

flink-clients/src/main/java/org/apache/flink/client/program/rest/RestClusterClient.java

 	//-------------------------------------------------------------------------

-	private <M extends MessageHeaders<EmptyRequestBody, P, U>, U extends MessageParameters, P extends ResponseBody> CompletableFuture<P>
+	@VisibleForTesting


Why do we need to make this visible for testing? I'd like to separate production from testing code as much as possible.

tillrohrmann · 2019-08-07T12:43:47Z

flink-tests/src/test/java/org/apache/flink/test/recovery/BatchFineGrainedRecoveryITCase.java

+				1 + // this mapper
+				1 + // this mapper after PartitionNotFoundException
+				i + // previous mappers
+				i) // previous mappers after PartitionNotFoundException


This is not so trivial to follow.

Ok, we can try to simplify the test or you think this particular calculation could be rewritten?

No I think the calculation with the comments is quite descriptive. However, the calculation itself is quite complicated because of all the different restarts being caused by different events.

tillrohrmann · 2019-08-07T12:45:01Z

flink-tests/src/test/java/org/apache/flink/test/recovery/BatchFineGrainedRecoveryITCase.java

 	private static TestingMiniCluster miniCluster;

+	private static MiniClusterClient client;
+


Setting FORCE_PARTITION_RELEASE_ON_CONSUMPTION to false in line 173 is no longer needed.

tillrohrmann · 2019-08-07T12:54:36Z

flink-tests/src/test/java/org/apache/flink/test/recovery/BatchFineGrainedRecoveryITCase.java

+
+		if (client != null) {
+			client.shutdown();
+		}


Side comment, why not using the MiniClusterResource to set up the TestEnvironment?

MiniClusterResource provides MiniCluster with protected methods: startTaskExecutor and terminateTaskExecutor used in this test. TestingMiniCluster exposes them as public.

All right. As a follow up one could think about providing a TestingMiniClusterResource but this is not necessary for this PR.

tillrohrmann · 2019-08-07T13:05:34Z

flink-tests/src/test/java/org/apache/flink/test/recovery/BatchFineGrainedRecoveryITCase.java

 		}
 	}
+
+	private static class MiniClusterClient extends RestClusterClient<StandaloneClusterId> {


Why do we need to extend the RestClusterClient? Wouldn't it be enough to simply use the RestClient with the rest address which one gets from the MiniCluster? Then one would not have to change the access modifiers of the RestClusterClient and not have to jump through all these hoops to set up the client configuration.

True, there is actually not so much code to reuse with the RestClusterClient.

azagrebin · 2019-08-07T15:13:27Z

Thanks for the review @tillrohrmann and the feedback about the test.
Let's discuss later how to simplify the test in general.
I addressed the comments and left a question about the expected attempt number calculation.

tillrohrmann

Thanks for updating this PR @azagrebin. LGTM. Merging this PR.

tillrohrmann · 2019-08-08T08:29:25Z

Hmm the japicmp plugin complains about the added methods to the SupplierWithException interface. I'll move them again to the Utils class.

If counting of mapper restarts in BatchFineGrainedRecoveryITCase is based on the open method of user function, the fact of the restart depends on internal implementation of the local Task and whether the open method is eventually called. If execution attempt numbers are used instead, the test behaviour is more stable because it depends only on coordination. The execution attempt numbers can be queried from the REST client of the testing mini cluster. This closes apache#9374.

tillrohrmann · 2019-08-08T09:07:59Z

Actually, we don't need 0e09c48. Using CompletableFuture#join does the trick.

If counting of mapper restarts in BatchFineGrainedRecoveryITCase is based on the open method of user function, the fact of the restart depends on internal implementation of the local Task and whether the open method is eventually called. If execution attempt numbers are used instead, the test behaviour is more stable because it depends only on coordination. The execution attempt numbers can be queried from the REST client of the testing mini cluster. This closes apache#9374.

If counting of mapper restarts in BatchFineGrainedRecoveryITCase is based on the open method of user function, the fact of the restart depends on internal implementation of the local Task and whether the open method is eventually called. If execution attempt numbers are used instead, the test behaviour is more stable because it depends only on coordination. The execution attempt numbers can be queried from the REST client of the testing mini cluster. This closes #9374.

If counting of mapper restarts in BatchFineGrainedRecoveryITCase is based on the open method of user function, the fact of the restart depends on internal implementation of the local Task and whether the open method is eventually called. If execution attempt numbers are used instead, the test behaviour is more stable because it depends only on coordination. The execution attempt numbers can be queried from the REST client of the testing mini cluster. This closes apache#9374.

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

azagrebin force-pushed the FLINK-13581 branch from 1b9f6d0 to a706bbd Compare August 6, 2019 15:10

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

tillrohrmann self-assigned this Aug 7, 2019

tillrohrmann requested changes Aug 7, 2019

View reviewed changes

azagrebin force-pushed the FLINK-13581 branch from a706bbd to bf7f688 Compare August 7, 2019 15:09

azagrebin force-pushed the FLINK-13581 branch from bf7f688 to 69613de Compare August 7, 2019 15:15

tillrohrmann approved these changes Aug 8, 2019

View reviewed changes

tillrohrmann force-pushed the FLINK-13581 branch from 69613de to 03733d7 Compare August 8, 2019 09:08

tillrohrmann closed this in c24b9a3 Aug 8, 2019

		private static TestingMiniCluster miniCluster;

		private static MiniClusterClient client;

[FLINK-13581][coordination][tests] Harden BatchFineGrainedRecoveryITCase #9374

[FLINK-13581][coordination][tests] Harden BatchFineGrainedRecoveryITCase #9374

Uh oh!

Conversation

azagrebin commented Aug 6, 2019

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

Uh oh!

flinkbot commented Aug 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 Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

tillrohrmann left a comment

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 Aug 8, 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!

Choose a reason for hiding this comment

Uh oh!

azagrebin commented Aug 7, 2019

Uh oh!

tillrohrmann left a comment

Choose a reason for hiding this comment

Uh oh!

tillrohrmann commented Aug 8, 2019

Uh oh!

tillrohrmann commented Aug 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flinkbot commented Aug 6, 2019 •

edited

Loading

flinkbot commented Aug 6, 2019 •

edited

Loading

tillrohrmann Aug 8, 2019 •

edited

Loading