Myasuka commented on a change in pull request #16582:
URL: https://github.com/apache/flink/pull/16582#discussion_r678241150
##########
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:
Thanks for your suggestion, I will consider to change this.
##########
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:
If we also send notification of subsumed savepoint, there would be a
problem that we might subsume a savepoint and a checkpoint in the same turn
when adding a new checkpoint. For this case, which checkpoint id should we
notify?
I see your PR of https://github.com/apache/flink/pull/16575 also consider
the savepoint case on TM side, was that the reason you prefer to send
notification of savepoints?
--
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]
