1996fanrui commented on code in PR #27050:
URL: https://github.com/apache/flink/pull/27050#discussion_r2424898467
##########
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java:
##########
@@ -1385,8 +1385,8 @@ private void completePendingCheckpoint(PendingCheckpoint
pendingCheckpoint)
lastSubsumed = null;
}
-
pendingCheckpoint.getCompletionFuture().complete(completedCheckpoint);
Review Comment:
> Deadlock / race condition if reportCompletedCheckpoint would trigger any
handler that also waits on the checkpoint future before its completion (in
general, unlikely situation, and should be caught by existing test)
Yes, it is a unlike situation. `reportCompletedCheckpoint` only has one
parameter, which is `completedCheckpoint`, so `reportCompletedCheckpoint` is
unable to access `pendingCheckpoint.getCompletionFuture()`.
> Checkpoint completion will be slightly delayed, but reporting is a quick
operation, so doesn't seem to be critical
Generally, both of them are quick. Of course, `complete` a
`CompletableFuture` is super quick.
> If reporting throws exception it will result in checkpoint being completed
exceptionally. Could we confirm that this behaviour matches the previous one?
Judging from the code, the behavior will definitely change for this case.
But I think the new behavior makes more sense.
Before this PR, the `CompletableFuture` is completed even if it is not
reported or report is failed. It causes wrong semantic, client receives the
checkpoint 10(or X) is completed, then get nothing when fetch more metadata for
checkpoint 10. (That is why MapStateNullValueCheckpointingITCase fails
occasionally)
After this PR, client could fetch the correct result once the client
received the complete signal.
The semantic will be clearer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
