pnowojski commented on a change in pull request #12714:
URL: https://github.com/apache/flink/pull/12714#discussion_r442952328
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/taskmanager/TaskTest.java
##########
@@ -871,6 +901,9 @@ public void
testTerminationFutureCompletesOnNormalExecution() throws Exception {
assertFalse(task.getTerminationFuture().isDone());
+ task.getExecutingThread().join(500); // allow some arbitrary
delay for potential termination and re-check
+ assertFalse(task.getTerminationFuture().isDone());
Review comment:
I'm not sure if I understand this part. Was this 500ms delay inducing a
failure in this test in the previous version if this PR? If not, I don't think
adding such sleep would be justified.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/taskmanager/TaskTest.java
##########
@@ -139,6 +142,33 @@ public void testRegularExecution() throws Exception {
taskManagerActions.validateListenerMessage(ExecutionState.FINISHED, task, null);
}
+ @Test
+ public void testTaskTerminatesOnlyAfterInvokable() throws Exception {
Review comment:
Is it covering for the user reported bug? It's not using
`SourceStreamTask` anywhere, so it looks like it's not? If that's the case,
couldn't we add a test in `SourceStreamTaskTest`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
