[GitHub] [flink] Myasuka commented on a change in pull request #16969: [FLINK-23949][runtime][checkpoint] fix first incremental checkpoint degenerate into full checkpoint after savepont

[GitBox <gi...@apache.org>]

Myasuka commented on a change in pull request #16969:
URL: https://github.com/apache/flink/pull/16969#discussion_r696251630



##########
File path: flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/snapshot/RocksIncrementalSnapshotStrategy.java
##########
@@ -188,7 +188,8 @@ public IncrementalRocksDBSnapshotResources syncPrepareResources(long checkpointI
     @Override
     public void notifyCheckpointComplete(long completedCheckpointId) {
         synchronized (materializedSstFiles) {
-            if (completedCheckpointId > lastCompletedCheckpointId) {
+            if (completedCheckpointId > lastCompletedCheckpointId
+                    && materializedSstFiles.keySet().contains(completedCheckpointId)) {

Review comment:
       This solution to this problem actually is to distinguish the notified checkpoint id belonging to a checkpooint or a savepoint. This can change the interface of `notifyCheckpointComplete` to include information of `CheckpointType` or let the state backend remeber which are checkpoints or savepoints. The former one needs to  change a lot of interfaces and we could currently choose the latter one.
   Actually, the additional statement of `materializedSstFiles.keySet().contains(completedCheckpointId)` just check whether the notified checkpointId is a checkpoint. I think we should add some comment to describe this.
   
   Moreover, I prefer to add a uinit test over `RocksIncrementalSnapshotStrategy` to verifty the logic:
   1. prepare a rocksDB, put some data and then call `syncPrepareResources`, `asyncSnapshot` and `notifyCheckpointComplete` on checkpoint-1.
   2. just `notifyCheckpointComplete` the `RocksIncrementalSnapshotStrategy` with savepoint-2
   3. again call `syncPrepareResources`, `asyncSnapshot` and `notifyCheckpointComplete` on checkpoint-3 to see whether the checkpoint is incremental.




-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org