linliu-code commented on code in PR #20028:
URL: https://github.com/apache/hudi/pull/20028#discussion_r4128891554
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java:
##########
@@ -618,20 +740,77 @@ 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 storage now holds the expired version of the lock we are
releasing, i.e. whether an
+ * earlier expire attempt that was answered with an error actually landed.
+ */
+ private boolean earlierExpireWriteLanded() {
+ Pair<LockGetResult, Option<StorageLockFile>> current =
storageLockClient.readCurrentLockFile();
Review Comment:
Addressed in the PR changes: the reconcile read catches runtime storage
errors and returns false, so the existing acquired-by-others path clears the
local lock and reports release failure. Added
testUnlockClearsLocalLockWhenReconcileReadThrows.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java:
##########
@@ -593,6 +680,12 @@ synchronized ExpireLockResult tryExpireCurrentLock(boolean
fromShutdownHook) {
logWarnLockState(FAILED_TO_RELEASE, "Lock expiration write was
throttled.");
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockThrottledMetric);
return ExpireLockResult.THROTTLED;
+ case TRANSIENT_ERROR:
+ // The storage backend rejected the write server-side (HTTP 5xx), so
the lock file is
+ // unchanged and still ours. Retrying the same conditional write is
safe.
Review Comment:
Addressed in follow-up commit 3bdc6d3. After a retriable release write, a
retry precondition failure now triggers a reread; it is treated as released
only when the stored lock is expired and still matches this owner and lease.
The existing failure path remains for another owner, and the reconcile-read
exception path now clears local state and reports release failure. Added tests
cover both landed-write reconciliation and a throwing reconcile read.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java:
##########
@@ -593,12 +698,29 @@ synchronized ExpireLockResult
tryExpireCurrentLock(boolean fromShutdownHook) {
logWarnLockState(FAILED_TO_RELEASE, "Lock expiration write was
throttled.");
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockThrottledMetric);
return ExpireLockResult.THROTTLED;
+ case TRANSIENT_ERROR:
+ // HTTP 5xx. Retrying the same conditional write is safe; if this
attempt did land despite
+ // the error, the retry's precondition failure is reconciled in the
ACQUIRED_BY_OTHERS case.
+ logWarnLockState(FAILED_TO_RELEASE, "Lock expiration write hit a
retriable server-side error.");
+
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockTransientErrorMetric);
+ return ExpireLockResult.TRANSIENT_ERROR;
case SUCCESS:
logInfoLockState(RELEASED);
recordAuditOperation(AuditOperationState.END, lockExpirationTimeMs);
setLock(null);
return ExpireLockResult.SUCCESS;
case ACQUIRED_BY_OTHERS:
+ if (afterRetriableAttempt && earlierExpireWriteLanded()) {
Review Comment:
I am preserving failure behavior when another owner holds the lock, per the
requested correctness constraint. A foreign owner is not sufficient proof of
our earlier expire write; reconciliation therefore accepts only our own expired
lock and lets the existing failure path handle a different owner.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java:
##########
@@ -618,20 +711,79 @@ 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.
+ * Renews (heartbeats) the current lock if we are the holder.
+ *
+ * <p>On a retriable storage rejection (throttling or a 5xx) this makes one
bounded extra
+ * attempt within the same heartbeat cycle before giving up and leaving the
rest to the next
+ * cycle. The backoff sleep happens outside the provider monitor so the
heartbeat thread does
+ * not hold it while waiting, which would stall {@link #unlock()} and {@link
#tryLock()}.
*
- * @return True if we successfully renewed the lock, false if not.
+ * @return True if the lock lease is still believed to be ours, false if
renewal should stop.
+ */
+ @VisibleForTesting
+ protected boolean renewLock() {
+ for (int attempt = 0; attempt < RENEW_MAX_RETRIES; attempt++) {
+ RenewAttemptOutcome outcome = renewLockOnce();
+ if (outcome.result != RenewAttemptOutcome.Result.RETRIABLE_FAILURE) {
+ return outcome.renewed;
+ }
+ long delaySeconds = THROTTLE_INITIAL_RETRY_DELAY_SECONDS;
+ logger.warn("Owner {}: Lock renewal hit a retriable storage error (retry
{}/{}), "
+ + "backing off for {} seconds.", ownerId, attempt + 1,
RENEW_MAX_RETRIES, delaySeconds);
+ try {
+ sleepForThrottleRetry(delaySeconds);
+ } catch (InterruptedException ie) {
+ // The heartbeat thread was asked to stop. Keep the lease (we still
believe it is ours)
+ // and let the shutdown path expire the lock.
+ Thread.currentThread().interrupt();
+ logger.warn("Owner {}: Interrupted while backing off before retrying
lock renewal.", ownerId, ie);
+
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockInterruptedMetric);
+ return true;
+ }
+ }
+ // Budget exhausted within this cycle; the lease is untouched, so let the
next heartbeat try.
+ return renewLockOnce().renewed;
+ }
+
+ /**
+ * Outcome of a single {@link #renewLockOnce()} pass: whether the caller
should keep the
Review Comment:
I kept the acquire and renew outcome types separate. Acquire distinguishes
acquired, ordinary non-acquisition, and retriable failure; renewal
distinguishes keep-heartbeat, stop-heartbeat, and retry. A shared type would
make the call sites less clear, and the later review agreed with keeping them
separate.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java:
##########
@@ -79,7 +79,9 @@ public class StorageBasedLockProvider implements
LockProvider<StorageLockFile> {
// However, since our lock leases are pretty long, we can use a high buffer.
private static final long CLOCK_DRIFT_BUFFER_MS = 500;
- // Max number of retry attempts on the lock-expire write after a THROTTLED
response.
+ // Max number of retry attempts on the lock-expire write after a retriable
response
+ // (THROTTLED or TRANSIENT_ERROR). Kept under the historical name because
the release path's
+ // budget has not changed.
Review Comment:
Addressed in the PR changes: retry budgets and backoff helpers now use
neutral names because they cover all retriable outcomes.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
