hudi-agent commented on code in PR #20028:
URL: https://github.com/apache/hudi/pull/20028#discussion_r4127107583
##########
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:
🤖 The reconcile only matches when storage still holds our expired lock.
Waiting writers poll every 1s
(`DEFAULT_LOCK_ACQUIRE_RETRY_WAIT_TIME_IN_MILLIS`), and our first retry is also
at +1s, so under contention another writer will often grab the released lock
before the retry. That case still raises the steal alert and FAILED_TO_RELEASE.
Would it be worth also accepting a foreign owner when our own lease
(`getLock().getValidUntilMs()`) had not yet elapsed? Another writer could only
have acquired if our expire landed.
<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 +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:
🤖 `readCurrentLockFile()` can throw here, for example `SdkClientException`
or `HoodieIOException` from S3. If it does, the exception escapes before
`setLock(null)`, which this ACQUIRED_BY_OTHERS branch always used to run.
`LockManager.unlock()` then skips `close()`, so the next `tryLock()` on the
reused provider takes the reentrant path and reports the lock as held. Could
you catch exceptions here, log them, and return false so it falls through to
the existing handling?
<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]
