[FLINK-15936] Harden TaskExecutorTest#testSlotAcceptance #11667
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test called taskSlotTable.allocateSlot from the test main thread, concurrently with taskSlotTable.createSlotReport while trying to register RM in the main TM thread. This silently failed the RM registration in TM.runAsync. As a result, RM.notifySlotAvailable was not called in TM. The commmit refactors the test to wait properly for RM registration and allocate slots through gateway in TM thread. The commit also uses proper testing RM/JM instead of mocks. To verify, the test has been looped locally 30k times.
Collaborator
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 475a7a5 (Wed Apr 08 07:45:21 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. DetailsThe 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 commandsThe @flinkbot bot supports the following commands:
|
zentol
reviewed
Apr 9, 2020
Contributor
zentol
left a comment
zentol
left a comment
There was a problem hiding this comment.
What about the calls to taskSlotTable#tryMarkSlotActive? Similar checks were removed in FLINK-14742.
flink-runtime/src/test/java/org/apache/flink/runtime/taskexecutor/TaskExecutorTest.java
Outdated
Show resolved
Hide resolved
Contributor
Author
|
Other calls technically happen when the state is synced with the test thread. |
zentol
reviewed
Apr 9, 2020
| // submit the task without having acknowledge the offered slots | ||
| return Either.Left(tmGateway.submitTask(tdd, jobMasterGateway.getFencingToken(), timeout).join()); | ||
| } catch (CompletionException e) { | ||
| return Either.Right(e.getCause()); |
Contributor
There was a problem hiding this comment.
Why not just let the cause bubble up, possibly enriching it?
You could then use assertThrows for checking the exception.
zentol
reviewed
Apr 9, 2020
| .build(); | ||
|
|
||
| try { | ||
| // submit the task without having acknowledge the offered slots |
Contributor
There was a problem hiding this comment.
this isn't necessarily true is it?
azagrebin
added a commit
that referenced
this pull request
Apr 14, 2020
The test called taskSlotTable.allocateSlot from the test main thread, concurrently with taskSlotTable.createSlotReport while trying to register RM in the main TM thread. This silently failed the RM registration in TM.runAsync. As a result, RM.notifySlotAvailable was not called in TM. The commmit refactors the test to wait properly for RM registration and allocate slots through gateway in TM thread. The commit also uses proper testing RM/JM instead of mocks. To verify, the test has been looped locally 30k times. This closes #11667.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
Concurrent
TaskSlotTableaccess in the unstable testThe test called
taskSlotTable.allocateSlotfrom the test main thread,concurrently with
taskSlotTable.createSlotReportwhile trying to registerRM in the main TM thread. This silently failed the RM registration in
TM.runAsync.As a result, RM.notifySlotAvailable was not called in TM.
The taskSlotTable is not thread-safe and must be accessed only from the main RPC thread of TM.
Failure in
establishResourceManagerConnectionFailure of
TaskExecutor#establishResourceManagerConnectionis not expected.It completely breaks the connection mechanism to RM in TM.
As a hotfix, the PR suggests to log it on error level at least.
Alternatively, we can consider calling
TM.onFatalErroras a follow-up.Brief change log
The PR refactors the test to wait properly for RM registration
and allocate slots through gateway in TM thread.
The PR also uses proper testing RM/JM instead of mocks.
Verifying this change
To verify, the test has been looped locally 30k times.