1996fanrui commented on code in PR #21547:
URL: https://github.com/apache/flink/pull/21547#discussion_r1070226451
##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/snapshot/RocksDBSnapshotStrategyBase.java:
##########
@@ -325,6 +329,7 @@ protected RocksDBSnapshotOperation(
this.checkpointStreamFactory = checkpointStreamFactory;
this.stateMetaInfoSnapshots = stateMetaInfoSnapshots;
this.localBackupDirectory = localBackupDirectory;
+ this.tmpResourcesRegistry = new CloseableRegistry();
Review Comment:
Hi @fredia , thanks a lot for your review.
As I understand, the `tmpResourcesRegistry` cannot be reused by multiple
checkpoints. If `tmpResourcesRegistry` is reused, it will be closed after chk-1
timeout, and chk-2 will call `tmpResourcesRegistry.registerCloseable()` after
writing file. However, chk-2 will throw `IOException` even if chk-2 is fine.
Current PR doesn't reuse the `tmpResourcesRegistry` by multiple checkpoints,
the `RocksDBSnapshotOperation` is created for single checkpoint, the
constructor includes the checkpointId.
The `testUploadedSstCanBeCleanedUp` reuses `tmpResourcesRegistry` by
multiple `uploader.uploadFilesToCheckpointFs`, it isn't reused by multiple
checkpoints. A single checkpoint may call `uploader.uploadFilesToCheckpointFs`
multiple times, for example: `RocksIncrementalSnapshotStrategy#uploadSstFiles`.
Please correct me if I'm wrong, thanks~ 🤔
<img width="995" alt="image"
src="https://user-images.githubusercontent.com/38427477/212459586-7c7327a4-1b8c-4dcb-a16a-aa5bb56b9443.png">
--
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]