[GitHub] [hudi] xushiyan 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]

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


##########
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:
   @watermelon12138 this does not look like a bug, can you change the jira type 
from bug to improvement? And let's hold off this PR before @yihua confirms on 
the validation scenario.



##########
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:
   this is a behavior change. @yihua do you know why this validation in the 
first place?



-- 
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-unsubscr...@hudi.apache.org

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