[GitHub] [hudi] yihua commented on a diff in pull request #5688: [HUDI-4157] fix Problem that IllegalArgumentException is thrown when the rollback is executed for a completed commit.

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

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


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackActionExecutor.java:
##########
@@ -88,11 +88,11 @@ public BaseRollbackActionExecutor(HoodieEngineContext context,
     this.resolvedInstant = instantToRollback;
     this.deleteInstants = deleteInstants;
     this.skipTimelinePublish = skipTimelinePublish;
-    this.useMarkerBasedStrategy = useMarkerBasedStrategy;
-    if (useMarkerBasedStrategy) {
-      ValidationUtils.checkArgument(!instantToRollback.isCompleted(),
-          "Cannot use marker based rollback strategy on completed instant:" + instantToRollback);
+    if (instantToRollback.isCompleted() && useMarkerBasedStrategy) {
+      useMarkerBasedStrategy = false;
+      LOG.warn("Cannot use marker based rollback strategy on completed instant:" + instantToRollback + ", the strategy has been automatically disabled.");

Review Comment:
   The validation is required because the marker-based rollback cannot be applied to a complete instant since there is no marker for complete instants.
   
   Instead of fixing the logic here to hide the anti-usage pattern, the caller of `BaseRollbackActionExecutor` should make sure that marker-based rollback is not used for completed instants.  @watermelon12138 could you clarify which UTs are relevant and why we need the fix here?



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