[GitHub] [flink-table-store] JingsongLi commented on a diff in pull request #75: [FLINK-27085] Introduce snapshot.num-retained.min option

[GitBox <gi...@apache.org>]

JingsongLi commented on code in PR #75:
URL: https://github.com/apache/flink-table-store/pull/75#discussion_r843625066


##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/file/FileStoreOptions.java:
##########
@@ -87,8 +87,14 @@
                             "The default partition name in case the dynamic partition"
                                     + " column value is null/empty string.");
 
-    public static final ConfigOption<Integer> SNAPSHOT_NUM_RETAINED =
-            ConfigOptions.key("snapshot.num-retained")
+    public static final ConfigOption<Integer> SNAPSHOT_NUM_RETAINED_MIN =
+            ConfigOptions.key("snapshot.num-retained.min")
+                    .intType()
+                    .defaultValue(10)
+                    .withDescription("The minimum number of completed snapshots to retain.");
+
+    public static final ConfigOption<Integer> SNAPSHOT_NUM_RETAINED_MAX =
+            ConfigOptions.key("snapshot.num-retained.max")

Review Comment:
   Please also modify ducument.



##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/file/operation/FileStoreExpireImpl.java:
##########
@@ -102,8 +105,8 @@ public void expire() {
         }
 
         // find the earliest snapshot to retain
-        for (long id = Math.max(latestSnapshotId - numRetained + 1, earliest);
-                id <= latestSnapshotId;

Review Comment:
   It seems `id < latestSnapshotId` is better in the past



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