akalash commented on a change in pull request #18845:
URL: https://github.com/apache/flink/pull/18845#discussion_r811289700
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpoint.java
##########
@@ -327,6 +331,9 @@ public static boolean checkpointsMatch(
*/
void setDiscardCallback(@Nullable CompletedCheckpointStats.DiscardCallback
discardCallback) {
this.discardCallback = discardCallback;
+ if (discardCallback != null && isDiscarded) {
+ discardCallback.notifyDiscardedCheckpoint();
+ }
Review comment:
1. It is a big question about the threading model of this class. First
of all, I see that the old field `discardCallback` was volatile. Secondly, I
see that `discard` method can be called from `CheckpointsCleaner` or
`CheckpointStore` which can be invoked from ExecutionThreadPool.
2. Triggering the callback is required because of what I described above if
that instance can be used in different threads we should be sure that the
callback will be called at least once(right now it is possible that it can be
called twice).
In general, I agree that the current implementation is more complicated. But
I didn't find an easier way. We can try to create `CompletedCheckpointStats` in
the same place where we did it before the changes but then we need somewhere to
hold it before reporting to `tracker`. I mean right now we do the reporting
here `PendingCheckpointStats#reportCompletedCheckpoint` but if we decided to
separate this method into two(creating, reporting) then we need to store
created object somewhere(CompletedCheckpoint maybe).
--
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]
