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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
