rkhachatryan commented on a change in pull request #17774:
URL: https://github.com/apache/flink/pull/17774#discussion_r768072754
##########
File path:
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/snapshot/RocksIncrementalSnapshotStrategy.java
##########
@@ -258,30 +268,37 @@ private SnapshotDirectory
prepareLocalSnapshotDirectory(long checkpointId) throw
}
Review comment:
> In this context by dead code I meant "not having any meaningful
effect". The code would be working just as well without this
notifyCheckpointAborted(), right? The materializedSstFiles would still be
cleaned up in notifyCheckpointComplete()?
Got you. Yes, correctness won't be affected by removing this method.
I agree that the naming is confusing. `materializedSstFiles` should probably
be called `uploadedStateIDs`. `lastUploadedSstFiles` can then be called
`uploadedStateHandles` (I've done this in
144aa68718e831cf07c2daa60b2d33ae149ce592).
WDYT?
I've also tried to combine the two maps into a single `SortedMap<Long,
Snapshot> snapshotsByCheckpoint`. But that didn't work because on checkpoint,
we might need data from potentially two checkpoints: last uploaded and last
confirmed.
--
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]
