[GitHub] [hudi] YuweiXiao commented on a diff in pull request #5681: [HUDI-4148] Preparations and client for hudi table manager service

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

YuweiXiao commented on code in PR #5681:
URL: https://github.com/apache/hudi/pull/5681#discussion_r882353269


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/compact/ScheduleCompactionActionExecutor.java:
##########
@@ -101,9 +105,14 @@ public Option<HoodieCompactionPlan> execute() {
       } catch (IOException ioe) {
         throw new HoodieIOException("Exception scheduling compaction", ioe);
       }
-      return Option.of(plan);
+      option = Option.of(plan);
     }
-    return Option.empty();
+
+    if (config.isTableManagerEnabled() && config.getTableManagerConfig().getTableManagerActions().contains(ActionType.compaction.name())) {

Review Comment:
   Maybe we could abstract this as an interface in config, e.g., isTableManagerIncludeAction(xxx)



##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanActionExecutor.java:
##########
@@ -236,6 +237,11 @@ private HoodieCleanMetadata runClean(HoodieTable<T, I, K, O> table, HoodieInstan
 
   @Override
   public HoodieCleanMetadata execute() {
+    if (config.isTableManagerEnabled() && config.getTableManagerConfig().getTableManagerActions().contains(ActionType.clean.name())) {
+      LOG.warn("Compaction delegate to table management service, do not clean for client!");

Review Comment:
   typo, -> 'clean delegate to'



##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/cluster/ClusteringPlanActionExecutor.java:
##########
@@ -102,6 +107,22 @@ public Option<HoodieClusteringPlan> execute() {
         throw new HoodieIOException("Exception scheduling clustering", ioe);
       }
     }
+
+    if (config.isTableManagerEnabled() && config.getTableManagerConfig().getTableManagerActions().contains(ActionType.replacecommit.name())) {
+      submitClusteringToService();
+    }
+
     return planOption;
   }
+
+  private void submitClusteringToService() {
+    HoodieTableMetaClient metaClient = table.getMetaClient();
+    List<String> instantsToSubmit = metaClient.getActiveTimeline()
+        .filterPendingReplaceTimeline()

Review Comment:
   Replace instant may also include `insert overwrite` operation. Will the manager service double-check to ensure it is a clustering instant?



##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/HoodieWriteMetadata.java:
##########
@@ -37,6 +37,8 @@
 
   // Will be set when auto-commit happens
   private boolean isCommitted;
+  // Will be set when table management service enable
+  private boolean isEmpty;

Review Comment:
   `isEmpty` seems a little bit confusing. How about `isSkipped`?



##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanActionExecutor.java:
##########
@@ -152,9 +156,26 @@ protected Option<HoodieCleanerPlan> requestClean(String startCleanTime) {
         LOG.error("Got exception when saving cleaner requested file", e);
         throw new HoodieIOException(e.getMessage(), e);
       }
-      return Option.of(cleanerPlan);
+      option = Option.of(cleanerPlan);
     }
-    return Option.empty();
+
+    if (config.isTableManagerEnabled() && config.getTableManagerConfig().getTableManagerActions().contains(ActionType.clean.name())) {
+      submitCleanToService();
+    }
+
+    return option;
+  }
+
+  private void submitCleanToService() {

Review Comment:
   These submit function will submit all pending clean/compaction/clustering instant to the service. What happens if we submit the same instant multiple times?



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org