[GitHub] [hudi] yihua commented on a diff in pull request #7035: [HUDI-5075] Adding support to rollback residual clustering after disabling clustering

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

yihua commented on code in PR #7035:
URL: https://github.com/apache/hudi/pull/7035#discussion_r1014445796


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -1335,10 +1335,14 @@ public boolean isClusteringEnabled() {
     return inlineClusteringEnabled() || isAsyncClusteringEnabled();
   }
 
-  public boolean isRollbackPendingClustering() {
+  public boolean isRollbackPendingClusteringOnConflict() {
     return getBoolean(HoodieClusteringConfig.ROLLBACK_PENDING_CLUSTERING_ON_CONFLICT);
   }
 
+  public boolean isRollbackPendingClustering() {
+    return getBoolean(HoodieClusteringConfig.ROLLBACK_PENDING_CLUSTERING);
+  }
+

Review Comment:
   Could you avoid changing the logic of existing APIs and rather adding a new API named `isRollbackPendingClusteringWhenDisablingClustering`?



##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -588,6 +588,19 @@ protected void runTableServicesInline(HoodieTable table, HoodieCommitMetadata me
         metadata.addMetadata(HoodieClusteringConfig.SCHEDULE_INLINE_CLUSTERING.key(), "true");
         inlineScheduleClustering(extraMetadata);
       }
+
+      // if clustering is disabled, but we might need to rollback any inflight clustering when clustering was enabled previously.
+      if (!config.inlineClusteringEnabled() && !config.isAsyncClusteringEnabled() && !config.scheduleInlineClustering()
+          && config.isRollbackPendingClustering()) {
+        // rollback any pending clustering
+        table.getActiveTimeline().filterPendingReplaceTimeline().getInstants().forEach(instant -> {
+          Option<Pair<HoodieInstant, HoodieClusteringPlan>> instantPlan = ClusteringUtils.getClusteringPlan(table.getMetaClient(), instant);
+          if (instantPlan.isPresent()) {
+            LOG.info("Rolling back pending clustering at instant " + instantPlan.get().getLeft());
+            rollback(instant.getTimestamp());

Review Comment:
   This API ignores the pending rollbacks for clustering.  For the following case, does it leave an inflight rollback in the timeline indefinitely?
   
   timeline: commit1, commit2, replacecommit3.requested (clustering), rollback1.inflight (rolling back replacecommit3, job failed in the middle)



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