[GitHub] [flink] Myasuka commented on a diff in pull request #21822: [FLINK-30863][state] Register local recovery files of changelog before notifyCheckpointComplete()

["Myasuka (via GitHub)" <gi...@apache.org>]

Myasuka commented on code in PR #21822:
URL: https://github.com/apache/flink/pull/21822#discussion_r1168984233


##########
flink-dstl/flink-dstl-dfs/src/main/java/org/apache/flink/changelog/fs/DuplicatingStateChangeFsUploader.java:
##########
@@ -51,14 +52,15 @@
  *       <li>Store the meta of files into {@link ChangelogTaskLocalStateStore} by
  *           AsyncCheckpointRunnable#reportCompletedSnapshotStates().
  *       <li>Pass control of the file to {@link LocalChangelogRegistry#register} when
- *           ChangelogKeyedStateBackend#notifyCheckpointComplete() , files of the previous
- *           checkpoint will be deleted by {@link LocalChangelogRegistry#discardUpToCheckpoint} at
- *           the same time.
+ *           FsStateChangelogWriter#persist , files of the previous checkpoint will be deleted by
+ *           {@link LocalChangelogRegistry#discardUpToCheckpoint} when the previous checkpoint is
+ *           confirmed.

Review Comment:
   I think there exists a serious problem for this solution. What will happen if no checkpoint could complete? It seems the local files would not be cleaned forever.



##########
flink-dstl/flink-dstl-dfs/src/test/java/org/apache/flink/changelog/fs/FsStateChangelogWriterTest.java:
##########
@@ -246,15 +248,233 @@ void testFileAvailableAfterClose() throws Exception {
         }
     }
 
+    @Test
+    void testLocalFileDiscard() throws Exception {

Review Comment:
   I tried to revert the code changes in this PR, this unit test could still pass. It seems the added test is useless.



-- 
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