Skip to content

[FLINK-11364][tests] Port TaskManagerFailsITCase to new code base #7676

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.

Sign up for GitHub

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

Jump to bottom
Closed
tillrohrmann wants to merge 4 commits into from
Closed

[FLINK-11364][tests] Port TaskManagerFailsITCase to new code base #7676

tillrohrmann wants to merge 4 commits into from

Conversation

@tillrohrmann
Copy link
Contributor

@tillrohrmann tillrohrmann commented Feb 11, 2019

What is the purpose of the change

Port TaskManagerFailsITCase to new code base.

Brief change log

  • "detect a failing task manager" --> JobMaster#testHeartbeatTimeoutWithTaskManager

  • "handle gracefully failing task manager" --> JobMasterTest#testJobFailureWhenGracefulTaskExecutorTermination

  • "handle hard failing task manager" --> JobMasterTest#testJobFailureWhenTaskExecutorHeartbeatTimeout

  • "go into a clean state in case of a TaskManager failure" --> TaskExecutorITCase#testNewTaskExecutorJoinsCluster

Verifying this change

  • Run ported tests

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
Copy link
Collaborator

flinkbot commented Feb 11, 2019 •
edited
Loading

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.

Review Progress

  • ✅ 1. The [description] looks good.
    • Approved by @GJL [committer]
  • ✅ 2. There is [consensus] that the contribution should go into to Flink.
    • Approved by @GJL [committer]
  • ❔ 3. Needs [attention] from.
  • ✅ 4. The change fits into the overall [architecture].
    • Approved by @GJL [committer]
  • ✅ 5. Overall code [quality] is good.
    • Approved by @GJL [committer]

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

Details

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot approve description to approve the 1st aspect (similarly, it also supports the consensus, architecture and quality keywords)
  • @flinkbot approve all to approve all aspects
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval

@GJL GJL self-requested a review February 11, 2019 16:13
@GJL GJL self-assigned this Feb 11, 2019
@GJL
Copy link
Member

GJL commented Feb 11, 2019

@flinkbot approve description
@flinkbot approve consensus

GJL
GJL requested changes Feb 11, 2019
View reviewed changes
flink-runtime/src/test/java/org/apache/flink/runtime/minicluster/TestingMiniCluster.java Outdated
}

@Override
public void startTaskExecutor(boolean localCommunication) throws Exception {
Copy link
Member

@GJL GJL Feb 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like user friendly API. Shouldn't MiniCluster set this flag depending on the configuration? Wouldn't it be enough to expose a signature such as public void startTaskExecutor()

Copy link
Contributor Author

@tillrohrmann tillrohrmann Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, MiniCluster cannot know whether to use local communication or not at start time, because TestingMiniCluster allows to start new TaskExecutors. Thus, the only option would be to always set localCommunication to false in the case of the MiniCluster.

Copy link
Contributor Author

@tillrohrmann tillrohrmann Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I introduced a new method useLocalCommunication which can be overriden by the TestingMiniCluster to always set local communication to false.

@tillrohrmann
Copy link
Contributor Author

tillrohrmann commented Feb 12, 2019

Thanks for the review @GJL. I've addressed your comments and pushed a fixup.

@tillrohrmann tillrohrmann mentioned this pull request Feb 12, 2019
Closed
@tillrohrmann tillrohrmann force-pushed the FLINK-11364 branch 2 times, most recently from e921816 to 248f105 Compare February 12, 2019 21:07
@tillrohrmann tillrohrmann mentioned this pull request Feb 13, 2019
Closed
GJL
GJL reviewed Feb 13, 2019
View reviewed changes
flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java Outdated
}

private void runJobFailureWhenTaskExecutorTerminatesTest(
Supplier<HeartbeatServices> heartbeatSupplier,
Copy link
Member

@GJL GJL Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does HeartbeatServices have to be lazily supplied?

Suggested change
Supplier<HeartbeatServices> heartbeatSupplier,
HeartbeatServices heartbeatServices,

Copy link
Contributor Author

@tillrohrmann tillrohrmann Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not correct. Will change.

flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java Outdated
jobGraph,
haServices,
new TestingJobManagerSharedServicesBuilder().build(),
heartbeatSupplier.get(),
Copy link
Member

@GJL GJL Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
heartbeatSupplier.get(),
heartbeatServices,

flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java Outdated
public void testJobFailureWhenTaskExecutorHeartbeatTimeout() throws Exception {
final AtomicBoolean respondToHeartbeats = new AtomicBoolean(true);
runJobFailureWhenTaskExecutorTerminatesTest(
() -> fastHeartbeatServices,
Copy link
Member

@GJL GJL Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
() -> fastHeartbeatServices,
fastHeartbeatServices,

flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java Outdated
@Test
public void testJobFailureWhenGracefulTaskExecutorTermination() throws Exception {
runJobFailureWhenTaskExecutorTerminatesTest(
() -> heartbeatServices,
Copy link
Member

@GJL GJL Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
() -> heartbeatServices,
heartbeatServices,

flink-runtime/src/test/java/org/apache/flink/runtime/jobmaster/JobMasterTest.java
private static final class TestingOnCompletionActions implements OnCompletionActions {

private final CompletableFuture<ArchivedExecutionGraph> jobReachedGloballyTerminalStateFuture = new CompletableFuture<>();
private final CompletableFuture<Void> jobFinishedByOtherFuture = new CompletableFuture<>();
Copy link
Member

@GJL GJL Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unused field is for future extensions?

Copy link
Contributor Author

@tillrohrmann tillrohrmann Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

flink-runtime/src/main/java/org/apache/flink/runtime/minicluster/MiniCluster.java Outdated
* Create a new {@link TerminatingFatalErrorHandler} for the {@link TaskExecutor} with
* the given index.
*
* @param index into the {{@link #taskManagers}} collection to identify the correct {@link TaskExecutor}.
Copy link
Member

@GJL GJL Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the double curly braces {{}} intended?

Copy link
Contributor Author

@tillrohrmann tillrohrmann Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unintended. will change it.

flink-runtime/src/test/java/org/apache/flink/runtime/minicluster/TestingMiniCluster.java Outdated
}

@Override
public void startTaskExecutor(boolean localCommunication) throws Exception {
Copy link
Member

@GJL GJL Feb 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it crucial for testing to be able to set the right localCommunication flag? If yes, a method overload that sets localCommunication to false would have been enough but I am not insisting on it.

edit: alternatively always use false with no option to override (if possible)

Copy link
Contributor Author

@tillrohrmann tillrohrmann Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you suggesting to always start the TaskExecutors with localCommunication = false? Or only for the TestingMiniCluster? The latter should now be the case.

Copy link
Contributor Author

@tillrohrmann tillrohrmann Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this functionality was needed for #7690.

@tillrohrmann
Copy link
Contributor Author

tillrohrmann commented Feb 13, 2019

Thanks for the second round of review @GJL. I addressed your comments except for the last one for which I didn't understand your proposal yet.

@GJL
Copy link
Member

GJL commented Feb 13, 2019

@flinkbot approve all

@tillrohrmann
[hotfix] Remove TaskExecutorITCase
7e6e2b5 
TaskExecutorITCase is actually covered by TaskExecutorTest#testOfferSlotToJobMasterAfterTimeout.
@tillrohrmann
[hotfix] Remove mocking from TaskExecutorTest#testJobLeaderDetection
fdd1107
@tillrohrmann
[hotfix] Remove mocking from TaskExecutorTest#testHeartbeatSlotReporting
5cee963
@tillrohrmann
[FLINK-11364][tests] Port TaskManagerFailsITCase to new code base
2172ab3 
- "detect a failing task manager" --> JobMaster#testHeartbeatTimeoutWithTaskManager

- "handle gracefully failing task manager" --> JobMasterTest#testJobFailureWhenGracefulTaskExecutorTermination

- "handle hard failing task manager" --> JobMasterTest#testJobFailureWhenTaskExecutorHeartbeatTimeout

- "go into a clean state in case of a TaskManager failure" --> TaskExecutorITCase#testNewTaskExecutorJoinsCluster

This closes apache#7676.
@tillrohrmann
Copy link
Contributor Author

tillrohrmann commented Feb 13, 2019

Thanks for the review @GJL. Merging once Travis gives green light.

@tillrohrmann tillrohrmann deleted the FLINK-11364 branch February 13, 2019 14:58
asfgit pushed a commit that referenced this pull request Feb 13, 2019
@tillrohrmann
[FLINK-11364][tests] Port TaskManagerFailsITCase to new code base
2004286 
- "detect a failing task manager" --> JobMaster#testHeartbeatTimeoutWithTaskManager

- "handle gracefully failing task manager" --> JobMasterTest#testJobFailureWhenGracefulTaskExecutorTermination

- "handle hard failing task manager" --> JobMasterTest#testJobFailureWhenTaskExecutorHeartbeatTimeout

- "go into a clean state in case of a TaskManager failure" --> TaskExecutorITCase#testNewTaskExecutorJoinsCluster

This closes #7676.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GJL GJL GJL approved these changes

Assignees

@GJL GJL

Labels

component=Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tillrohrmann @flinkbot @GJL @rmetzger