[FLINK-13325][test] Add test to guard the fix for FLINK-13249 #9170
Conversation
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 846b50b (Tue Aug 06 15:51:31 UTC 2019) 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:
|
|
I created a test for the problem that is hopefully a good compromise between testing a rather low-level detail and not going to super deep in the IT case territory. Wdyt guys? CC @zhijiangW |
| public void testRegularExecution() throws Exception { | ||
| final QueuedNoOpTaskManagerActions taskManagerActions = new QueuedNoOpTaskManagerActions(); | ||
| final Task task = createTaskBuilder() | ||
| .setInvokable(TestInvokableCorrect.class) |
There was a problem hiding this comment.
This change might be a separate pre hotfix commit.
| // helper functions | ||
| // ------------------------------------------------------------------------ | ||
|
|
||
| private void setInputGate(Task task, SingleInputGate inputGate) { |
There was a problem hiding this comment.
nit: this legacy code remove could also be a separate commit.
|
|
||
| { | ||
| invokable = AbstractInvokable.class; | ||
| taskManagerActions = mock(TaskManagerActions.class); |
There was a problem hiding this comment.
It might use NoOpTaskManagerActions instead to avoid mock after refactoring.
| invokable = AbstractInvokable.class; | ||
| taskManagerActions = mock(TaskManagerActions.class); | ||
|
|
||
| libraryCacheManager = mock(LibraryCacheManager.class); |
There was a problem hiding this comment.
ditto: ContextClassLoaderLibraryCacheManager.INSTANCE to avoid mock.
| when(libraryCacheManager.getClassLoader(any(JobID.class))).thenReturn(getClass().getClassLoader()); | ||
|
|
||
| consumableNotifier = new NoOpResultPartitionConsumableNotifier(); | ||
| partitionProducerStateChecker = mock(PartitionProducerStateChecker.class); |
There was a problem hiding this comment.
We could make NoOpPartitionProducerStateChecker public and use this.
| new Configuration()); | ||
|
|
||
| final BlobCacheService blobCacheService = new BlobCacheService( | ||
| mock(PermanentBlobCache.class), |
There was a problem hiding this comment.
ditto: avoid mock if possible
| resultPartitions, | ||
| inputGates, | ||
| 0, | ||
| mock(MemoryManager.class), |
There was a problem hiding this comment.
ditto: MemoryManager and IOManager
|
|
||
| public static void setTaskState(Task task, ExecutionState state) { | ||
| try { | ||
| Field f = Task.class.getDeclaredField("executionState"); |
There was a problem hiding this comment.
Another option is that we make the method Task#transitionState as package private.
| Assert.assertFalse( | ||
| "Test ended by timeout or interruption - this indicates that the network thread was blocked.", | ||
| timedOutOrInterrupted.get()); | ||
| } |
There was a problem hiding this comment.
Might be better to close NettyShuffleEnvironment and SingleInputGate finally.
| /** | ||
| * Test to guard against FLINK-13249. | ||
| */ | ||
| @Test |
There was a problem hiding this comment.
Might be better not to put this test among utility methods.
zhijiangW
left a comment
zhijiangW
left a comment
There was a problem hiding this comment.
@StefanRRichter Thanks for opening this PR for covering the previous concerned bug fix.
This unit test looks good to me, only left some other nit comments.
|
I validated that the test indeed fails without the original fix. Will address @zhijiangW's comments and merge this. |
5 participants
What is the purpose of the change
This PR is only adding the missing test case to guard our fix for FLINK-13249.