hudi-agent commented on code in PR #18439:
URL: https://github.com/apache/hudi/pull/18439#discussion_r3184598317
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java:
##########
@@ -346,9 +346,21 @@ public synchronized boolean tryLock() {
newLockData,
latestLock.getRight());
if (lockUpdateStatus.getLeft() != LockUpsertResult.SUCCESS) {
- // failed to acquire the lock, indicates concurrent contention
logInfoLockState(FAILED_TO_ACQUIRE);
-
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockAcquirePreconditionFailureMetric);
+ switch (lockUpdateStatus.getLeft()) {
+ case ACQUIRED_BY_OTHERS:
+ // failed to acquire the lock, indicates concurrent contention
+
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockAcquirePreconditionFailureMetric);
+ break;
+ case THROTTLED:
Review Comment:
🤖 nit: the comment "Lock state is unknown after the upsert attempt" doesn't
quite fit the `THROTTLED` case — after a 429 the write was definitively
rejected, so we know we didn't acquire the lock. "Unknown state" is accurate
for `UNKNOWN_ERROR` but not for throttling. Could you split the comment, or
rephrase to something like "Transient failure — the lock was not acquired; may
succeed on retry"?
<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:
##########
@@ -424,22 +436,55 @@ private boolean believesLockMightBeHeld() {
* Unlock by marking our current lock file "expired": true.
*/
@Override
- public synchronized void unlock() {
- assertHeartbeatManagerExists();
- if (!believesLockMightBeHeld()) {
- return;
+ public void unlock() {
+ ExpireLockResult expireResult;
+ // Snapshot the exact StorageLockFile we intend to expire. Identity
comparison in the retry
+ // path guards against a concurrent tryLock() that, if our original lock
had expired during
+ // the 1s sleep, could have installed a fresh lock that the retry must NOT
mark expired.
+ StorageLockFile lockToExpire;
+ synchronized (this) {
+ assertHeartbeatManagerExists();
+ if (!believesLockMightBeHeld()) {
+ return;
+ }
+
+ // Try to stop the heartbeat first
+ if (heartbeatManager.hasActiveHeartbeat()) {
+ logger.debug("Owner {}: Gracefully shutting down heartbeat.", ownerId);
+ if (!heartbeatManager.stopHeartbeat(true)) {
+
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockReleaseFailureMetric);
+ throw new
HoodieLockException(generateLockStateMessage(FAILED_TO_RELEASE));
+ }
+ }
+
+ // Then expire the current lock.
+ lockToExpire = getLock();
+ expireResult = tryExpireCurrentLock(false);
}
- boolean believesNoLongerHoldsLock = true;
- // Try to stop the heartbeat first
- if (heartbeatManager.hasActiveHeartbeat()) {
- logger.debug("Owner {}: Gracefully shutting down heartbeat.", ownerId);
- believesNoLongerHoldsLock &= heartbeatManager.stopHeartbeat(true);
+ // If throttled, retry once after sleeping outside the monitor to avoid
blocking other threads.
+ // Note: when unlock() is called via close() -> shutdown(), the caller's
synchronized methods
Review Comment:
🤖 nit: "the sleep will still block in that path" is a bit ambiguous — it
reads as if the sleep is itself blocking, but the actual concern is that the
outer `synchronized close()`/`shutdown()` caller still holds the monitor
throughout the sleep. Could you rephrase to something like "the outer
synchronized caller still holds the monitor through the sleep, so other threads
remain blocked in that scenario"?
<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]
