linliu-code commented on code in PR #20028:
URL: https://github.com/apache/hudi/pull/20028#discussion_r4161414852
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java:
##########
@@ -367,22 +430,29 @@ public synchronized boolean tryLock() {
logInfoLockState(FAILED_TO_ACQUIRE);
switch (lockUpdateStatus.getLeft()) {
case ACQUIRED_BY_OTHERS:
- // failed to acquire the lock, indicates concurrent contention
+ // failed to acquire the lock, indicates concurrent contention.
Retrying immediately
+ // would not help — the other owner holds a valid lease.
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockAcquirePreconditionFailureMetric);
- break;
+ return AcquireAttemptResult.NOT_ACQUIRED;
case THROTTLED:
- // The write was rejected; we did not acquire. Transient — caller
may retry.
+ // The write was rejected; we did not acquire. Transient — worth a
bounded retry.
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockThrottledMetric);
- break;
+ return AcquireAttemptResult.RETRIABLE_FAILURE;
+ case TRANSIENT_ERROR:
+ // Storage returned a 5xx. The conditional write cannot clobber
another writer, so a
+ // bounded retry is safe. A 5xx does not prove the write was
rejected, though: if it
+ // landed, the retry finds our own untracked lock and reports it as
held until it lapses.
+
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockTransientErrorMetric);
Review Comment:
Yes, you're right. If a 5xx acquire write actually landed, the retry reads
our own valid lock and reports it as held. That lock has no heartbeat, so every
writer waits out the lease, the same as before this PR. The only case the inner
retry improves is a write that did not land, and the outer `tryLock(time,
unit)` loop already covered that one.
The fix is to read the lock back and adopt it when it carries our owner, is
still valid, and has the expiration we wrote. I've written that up, together
with the renew and release cases, as an RFC-91 update in #20186, so we can
agree on the rule before I rework this PR.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java:
##########
@@ -653,50 +850,89 @@ protected synchronized boolean renewLock() {
Option.of(getLock()));
switch (currentLock.getLeft()) {
case ACQUIRED_BY_OTHERS:
+ if (afterRetriableAttempt) {
Review Comment:
Agreed. With the gate, a landed renewal followed by a second 5xx is still
reported as a steal on the next cycle. #20186 drops the gate: the provider
remembers an unresolved write and reads the lock back on any later 412 until it
gets a definitive answer. This PR will implement that once #20186 is agreed.
--
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]
