rkhachatryan commented on a change in pull request #12714:
URL: https://github.com/apache/flink/pull/12714#discussion_r442782518
##########
File path:
flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/tasks/StreamTask.java
##########
@@ -657,20 +657,25 @@ protected void cleanUpInvoke() throws Exception {
}
@Override
- public final void cancel() throws Exception {
+ public final CompletableFuture<Void> cancel() {
isRunning = false;
canceled = true;
// the "cancel task" call must come first, but the cancelables
must be
// closed no matter what
- try {
- cancelTask();
- }
- finally {
- mailboxProcessor.allActionsCompleted();
- cancelables.close();
+ return FutureUtils
+ .completedVoidFuture()
+ .thenRun(this::cancelTask)
Review comment:
Because `cancelTask` can also throw an exception.
Leaving client code to deal with both method throwing an exception and
Future completed exceptionally isn't very nice IMO.
Having a `finally` block (I guess that's what you mean) would make calling
`mailboxProcessor.allActionsCompleted` more complicated (I think it should be
called after future completes if it was created successfully).
This is btw an idiomatic way in some languages like Scala.
----------------------------------------------------------------
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]
