danny0405 commented on code in PR #8506:
URL: https://github.com/apache/hudi/pull/8506#discussion_r1177219100


##########
hudi-common/src/main/java/org/apache/hudi/common/table/view/TableFileSystemView.java:
##########
@@ -194,6 +194,11 @@ interface SliceView extends SliceViewWithLatestSlice {
    */
   Stream<HoodieFileGroup> getReplacedFileGroupsBefore(String maxCommitTime, 
String partitionPath);
 
+  /**
+   * Stream all the replaced file groups alter or on minCommitTime.
+   */

Review Comment:
   alter or on -> after or on



##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##########
@@ -367,10 +367,11 @@ private Pair<Boolean, List<CleanFileInfo>> 
getFilesToCleanKeepingLatestCommits(S
           }
         }
       }
-      // if there are no valid file groups
+      // if all file groups ready to be cleaned
       // and no pending data files under the partition [IMPORTANT],
       // mark it to be deleted
-      if (fileGroups.isEmpty() && !hasPendingFiles(partitionPath)) {

Review Comment:
   ```java
         // if there are no valid file groups
         // and no pending data files under the partition [IMPORTANT],
         // and no subsequent replace commit after the earliest retained commit
         // mark it to be deleted
   ```



##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##########
@@ -367,10 +367,11 @@ private Pair<Boolean, List<CleanFileInfo>> 
getFilesToCleanKeepingLatestCommits(S
           }
         }
       }
-      // if there are no valid file groups
+      // if all file groups ready to be cleaned
       // and no pending data files under the partition [IMPORTANT],
       // mark it to be deleted
-      if (fileGroups.isEmpty() && !hasPendingFiles(partitionPath)) {
+      if (fileGroups.isEmpty() && !hasPendingFiles(partitionPath)
+              && 
!fileSystemView.getReplacedFileGroupsAfterOrOn(earliestCommitToRetain.getTimestamp(),
 partitionPath).findAny().isPresent()) {
         toDeletePartition = true;

Review Comment:
   Can we move the line 374 into a separate method, and line 268 also needs to 
be fixed.



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

Reply via email to