dawidwys commented on a change in pull request #15550:
URL: https://github.com/apache/flink/pull/15550#discussion_r610715281
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskExecutor.java
##########
@@ -932,9 +932,9 @@ public void heartbeatFromResourceManager(ResourceID
resourceID) {
final Task task = taskSlotTable.getTask(executionAttemptID);
if (task != null) {
- task.triggerCheckpointBarrier(checkpointId, checkpointTimestamp,
checkpointOptions);
-
- return CompletableFuture.completedFuture(Acknowledge.get());
+ return task.triggerCheckpointBarrier(
+ checkpointId, checkpointTimestamp,
checkpointOptions)
+ .thenApply(result -> Acknowledge.get());
Review comment:
Right, I am also not sure about the change. However if we go with
ignoring the result (option 2. from the description) we should revisit the
whole stack, as there is a somewhat bigger logic in
`org.apache.flink.streaming.runtime.tasks.MultipleInputStreamTask#pendingCheckpointCompletedFutures`
which does not work now.
Is it fine to drop it? Especially cancelling futures for the oldest pending
checkpoints? If it is fine to drop (it does not work anyway) then I am also +1
to drop the return value whatsoever.
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskExecutor.java
##########
@@ -932,9 +932,9 @@ public void heartbeatFromResourceManager(ResourceID
resourceID) {
final Task task = taskSlotTable.getTask(executionAttemptID);
if (task != null) {
- task.triggerCheckpointBarrier(checkpointId, checkpointTimestamp,
checkpointOptions);
-
- return CompletableFuture.completedFuture(Acknowledge.get());
+ return task.triggerCheckpointBarrier(
+ checkpointId, checkpointTimestamp,
checkpointOptions)
+ .thenApply(result -> Acknowledge.get());
Review comment:
Right, I am also not sure about the change. However if we go with
ignoring the result (option 2. from the description) we should revisit the
whole stack, as there is a somewhat bigger logic in
`org.apache.flink.streaming.runtime.tasks.MultipleInputStreamTask#pendingCheckpointCompletedFutures`
which does not work now. Is it fine to drop it? Especially cancelling futures
for the oldest pending checkpoints? If it is fine to drop (it does not work
anyway) then I am also +1 to drop the return value whatsoever.
--
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]
