rkhachatryan commented on a change in pull request #16582:
URL: https://github.com/apache/flink/pull/16582#discussion_r678215722
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
##########
@@ -1220,8 +1224,15 @@ private void completePendingCheckpoint(PendingCheckpoint
pendingCheckpoint)
Preconditions.checkState(pendingCheckpoint.isDisposed() &&
completedCheckpoint != null);
try {
- completedCheckpointStore.addCheckpoint(
- completedCheckpoint, checkpointsCleaner,
this::scheduleTriggerRequest);
+ CompletedCheckpoint lastSubsumed =
+
completedCheckpointStore.addCheckpointAndSubsumeOldestOne(
+ completedCheckpoint,
+ checkpointsCleaner,
+ this::scheduleTriggerRequest);
+ if (lastSubsumed != null && lastSubsumed.discardOnSubsume()) {
Review comment:
I think we shouldn't make such a distinction.
If TM needs to distinguish for some particular task (state management), it
can be done on TM side.
One practical reason to notify TMs is that subsuming a savepoint allows them
subsume older checkpoints.
A more general reason that the community I think is moving towards
miminizing the difference in handling of savepoints and checkpoints.
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
##########
@@ -229,6 +229,8 @@
private final ExecutionAttemptMappingProvider attemptMappingProvider;
+ @Nullable private CompletedCheckpoint lastSubsumedCheckpoint;
+
Review comment:
I think the added complexity in `CheckpointCoordinator` doesn't worth it
as it's a critical component.
And it's more valuable to test the actual functionality - i.e. what ID is
sent in the notifications.
--
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]
