rkhachatryan commented on code in PR #22669: URL: https://github.com/apache/flink/pull/22669#discussion_r1212443717
########## flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/snapshot/RocksDBSnapshotStrategyBase.java: ########## @@ -395,18 +394,16 @@ public void release() { /** Previous snapshot with uploaded sst files. */ protected static class PreviousSnapshot { - @Nullable private final Map<StateHandleID, Long> confirmedSstFiles; + @Nullable private final Map<StateHandleID, StreamStateHandle> confirmedSstFiles; - protected PreviousSnapshot(@Nullable Map<StateHandleID, Long> confirmedSstFiles) { + protected PreviousSnapshot( + @Nullable Map<StateHandleID, StreamStateHandle> confirmedSstFiles) { this.confirmedSstFiles = confirmedSstFiles; } protected Optional<StreamStateHandle> getUploaded(StateHandleID stateHandleID) { if (confirmedSstFiles != null && confirmedSstFiles.containsKey(stateHandleID)) { - // we introduce a placeholder state handle, that is replaced with the - // original from the shared state registry (created from a previous checkpoint) - return Optional.of( - new PlaceholderStreamStateHandle(confirmedSstFiles.get(stateHandleID))); Review Comment: Thanks for the explanation @zoltar9264 My concern is exactly about additional overhead of data transmission (the purpose of placeholder is to reduce it). I'm not sure whether `PlaceholderStreamStateHandle` removal will not cause a regression. It depends not only on the state size, but also on the configuration (`state.storage.fs.memory-threshold`) and the actual size of the SST files. So I think it would be safer to keep it. I agree that that requires more code changes, but I think it worth it. The complexity added by `PlaceholderStreamStateHandle` doesn't seem very high. WDYT? -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org