hudi-agent commented on code in PR #20028:
URL: https://github.com/apache/hudi/pull/20028#discussion_r4074037216
##########
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:
🤖 The PR description says a 5xx that silently landed converges via 412 →
`ACQUIRED_BY_OTHERS`, "which is already handled as released" — but here that
branch maps to `FAILED`, logs the "acquired by another owner … heartbeat
failure" error, bumps the acquired-by-others metric, skips the audit END, and
`unlock()` still throws `FAILED_TO_RELEASE`. Would it be worth, on a retry
attempt only, reading the file back on 412 and treating `expired && owner ==
ownerId` as SUCCESS so a landed 5xx doesn't surface as a misleading lock-steal
alert?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
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:
🤖 nit: `RenewAttemptOutcome` and the existing `AcquireAttemptResult` enum
both model the same "terminal vs retriable" concept independently — could be
worth sharing a single small result type between the acquire and renew paths to
avoid the duplicated RETRIABLE_FAILURE/TERMINAL concept.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
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:
🤖 nit: now that this budget also governs retries after a `TRANSIENT_ERROR`
(5xx), the `THROTTLE_MAX_RETRIES`/`sleepForThrottleRetry` names read as
throttle-only and could mislead a future reader. Might be worth a more neutral
name like `RETRIABLE_MAX_RETRIES`/`sleepForRetriableBackoff` instead of keeping
the historical throttle-specific name.
<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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
