[GitHub] [hudi] zhangyue19921010 commented on a change in pull request #3956: [HUDI-2641] Avoid deleting all inflight commits heartbeats while rolling back failed writes

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

zhangyue19921010 commented on a change in pull request #3956:
URL: https://github.com/apache/hudi/pull/3956#discussion_r746260110



##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java
##########
@@ -856,13 +856,9 @@ protected void rollbackFailedWrites(List<String> instantsToRollback, boolean ski
       } else {
         rollback(instant, skipLocking);
       }
-    }
-    // Delete any heartbeat files for already rolled back commits
-    try {
-      HeartbeatUtils.cleanExpiredHeartbeats(this.heartbeatClient.getAllExistingHeartbeatInstants(),
-          createMetaClient(true), basePath);
-    } catch (IOException io) {
-      LOG.error("Unable to delete heartbeat files", io);
+
+      // Delete the heartbeat for rolled back instant
+      HeartbeatUtils.deleteHeartbeatFile(fs, basePath, instant, config);

Review comment:
       In this patch, we delete the rollback instant in the `for (String instant : instantsToRollback)` loop. 
   Is there may something un-deleted when going into https://github.com/apache/hudi/blob/add8acecb367ee7b8506e5abda9137d203e34645/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java#L855 branch ?
   
   Maybe we could do the delete HeartbeatsFiles out of the loop :)

##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java
##########
@@ -856,13 +856,9 @@ protected void rollbackFailedWrites(List<String> instantsToRollback, boolean ski
       } else {
         rollback(instant, skipLocking);
       }
-    }
-    // Delete any heartbeat files for already rolled back commits
-    try {
-      HeartbeatUtils.cleanExpiredHeartbeats(this.heartbeatClient.getAllExistingHeartbeatInstants(),

Review comment:
       Did we need to remove these unused function like `getAllExistingHeartbeatInstants` and `cleanExpiredHeartbeats` after this patch?




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