1996fanrui commented on code in PR #24757: URL: https://github.com/apache/flink/pull/24757#discussion_r1612639848
########## flink-runtime/src/main/java/org/apache/flink/runtime/source/coordinator/SourceCoordinator.java: ########## @@ -201,8 +201,11 @@ void announceCombinedWatermark() { // to ready task to avoid period task fail (Java-ThreadPoolExecutor will not schedule // the period task if it throws an exception). for (Integer subtaskId : subTaskIds) { - context.sendEventToSourceOperatorIfTaskReady( - subtaskId, new WatermarkAlignmentEvent(maxAllowedWatermark)); + // when subtask have been finished, do not send event. + if (!context.hasNoMoreSplits(subtaskId)) { Review Comment: > I believe these two locations should be equivalent. I think you are right, both of `DataInputStatus.END_OF_INPUT` and `NoMoreSplitsEvent` work well for now. I prefer `DataInputStatus.END_OF_INPUT` because its semantics are clearer, and it's closer to FINISHED. It means the task will be definitely switched to FINISHED after `DataInputStatus.END_OF_INPUT`. As we all know, code is often refactored. We shouldn't make assumptions (especially assumptions that other developers don't know about). Other developers may break our assumption if they don't know during the refactor in the future. I'm afraid the `NoMoreSplitsEvent` is used for other cases, and task doesn't switch to FINISHED after receiving `NoMoreSplitsEvent` in the future. That's my concern. -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org