Re: [PR] fix(lock): retry an indeterminate lock-expire write [hudi]

["hudi-agent (via GitHub)" <gi...@apache.org>]

hudi-agent commented on code in PR #20129:
URL: https://github.com/apache/hudi/pull/20129#discussion_r4150598781


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java:
##########
@@ -618,20 +745,98 @@ synchronized ExpireLockResult tryExpireCurrentLock(boolean fromShutdownHook) {
   }
 
   /**
-   * Renews (heartbeats) the current lock if we are the holder, it forcefully set
-   * the expiration flag
-   * to false and the lock expiration time to a later time in the future.
+   * Whether an earlier expire attempt that was answered with an error actually landed: storage
+   * holds our expired lock, or another owner holds it while our lease has not yet elapsed.
+   */
+  private boolean earlierExpireWriteLanded() {
+    final Pair<LockGetResult, Option<StorageLockFile>> current;
+    try {
+      current = storageLockClient.readCurrentLockFile();
+    } catch (RuntimeException e) {
+      // A failed reconcile read does not change the original precondition failure. Let the
+      // normal ACQUIRED_BY_OTHERS handling clear our local lock and report release failure.
+      logger.warn("Owner {}: Failed to reconcile lock expiration after a retriable write error for {}.",
+          ownerId, lockFilePath, e);
+      return false;
+    }
+    if (current.getLeft() != LockGetResult.SUCCESS || !current.getRight().isPresent()) {
+      return false;
+    }
+    StorageLockFile stored = current.getRight().get();
+    if (ownerId.equals(stored.getOwner())) {
+      return stored.isExpired() && stored.getValidUntilMs() == getLock().getValidUntilMs();
+    }
+    // Others take over a live lease only after validUntil + CLOCK_DRIFT_BUFFER_MS, so another
+    // owner holding it inside our lease means our expire landed and they acquired after it.
+    return getCurrentEpochMs() < getLock().getValidUntilMs();

Review Comment:
   🤖 This only holds if the other writer's clock is within CLOCK_DRIFT_BUFFER_MS of ours. A writer whose clock runs ahead (the case the ACQUIRED_BY_OTHERS log below calls out), or a lock file someone deleted, would now be reported as a clean release with no acquired-by-others metric. Could this branch log at WARN with the other owner and how much of our lease was left, or count something, so a skew steal can still be diagnosed?
   
   <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub>



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