fix(lock): name the cause on FAILED_TO_RELEASE in StorageBasedLockProvider - #19574
Open
pkgajulapalli wants to merge 1 commit into
Open
fix(lock): name the cause on FAILED_TO_RELEASE in StorageBasedLockProvider#19574pkgajulapalli wants to merge 1 commit into
pkgajulapalli wants to merge 1 commit into
Conversation
…vider Three distinct failures in StorageBasedLockProvider#unlock() threw a byte-identical HoodieLockException message, so logs could not tell them apart. All three also share a single updateLockReleaseFailureMetric counter, leaving no way to attribute a release failure to a cause. Each throw now names its cause, and each logs the context needed to act on it: - HEARTBEAT_STOP_FAILED: the heartbeat task would not stop, so the lock is deliberately left un-expired (the task could still renew it after we return). Logs the interrupted flag to separate the two sub-cases in LockProviderHeartbeatManager#stopHeartbeat. - INTERRUPTED_DURING_THROTTLE_BACKOFF: interrupted mid-backoff. Now also passes the InterruptedException so the stack trace survives. - THROTTLE_RETRIES_EXHAUSTED vs EXPIRE_WRITE_FAILED: distinguishes an exhausted retry budget against a storage rate limit (e.g. the GCS 1-write/sec per-object limit) from a terminal UNKNOWN_ERROR / ACQUIRED_BY_OTHERS outcome. The four cause strings are declared as constants next to the other lock tunables, so the full set is visible in one place and both the call sites and the test assertions reference them rather than raw literals. On ACQUIRED_BY_OTHERS, also log how long ago our lease should have ended. A positive value means we overran our own lease, pointing at a starved heartbeat (long GC, thread-pool starvation); a negative value means the lease had not elapsed by our clock, pointing at clock skew between nodes instead. Those two causes are indistinguishable today and call for different fixes. Every new message carries lockFilePath, so a lock left dangling in storage can be joined back to the writer that failed to release it. Behaviour is unchanged: control flow is untouched, and each edit either adds a logger.error call or appends ", cause <LABEL>" to an existing exception message. Metrics are unchanged. Adds a test for the interrupted-during-backoff path, which had no coverage, and tightens the three existing FAILED_TO_RELEASE assertions to pin the specific cause label.
hudi-agent
reviewed
Aug 10, 2026
hudi-agent
left a comment
hudi-agent
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the contribution! This PR disambiguates the three distinct FAILED_TO_RELEASE failure paths in StorageBasedLockProvider#unlock() by naming each cause in the thrown HoodieLockException and adding targeted error logging. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. One small logging readability nit; otherwise clean.
cc @yihua
| // THROTTLED here means the retries above were exhausted; FAILED means tryExpireCurrentLock | ||
| // already logged the specific storage outcome (UNKNOWN_ERROR vs ACQUIRED_BY_OTHERS). | ||
| String cause = expireResult == ExpireLockResult.THROTTLED | ||
| ? CAUSE_THROTTLE_RETRIES_EXHAUSTED |
Contributor
There was a problem hiding this comment.
🤖 nit: cause is computed just above but then omitted from the log message — only expireResult (the enum) appears. Could you add cause as a placeholder so the cause string (e.g. THROTTLE_RETRIES_EXHAUSTED) is greppable directly in logs, without having to correlate with the exception? e.g. "... ended as {} (cause={}) after ..." with args expireResult, cause.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19574 +/- ##
============================================
+ Coverage 77.09% 77.10% +0.01%
- Complexity 32490 32500 +10
============================================
Files 2522 2522
Lines 139112 139126 +14
Branches 16714 16715 +1
============================================
+ Hits 107243 107276 +33
+ Misses 24291 24278 -13
+ Partials 7578 7572 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the issue this Pull Request addresses
StorageBasedLockProvider#unlock()has three distinct failure paths that all throw abyte-identical
HoodieLockExceptionmessage:All three also share a single
updateLockReleaseFailureMetriccounter. So when a lockrelease fails in production, neither the exception nor the metric tells you which path
produced it — and the causes call for completely different fixes (stop the writer's GC
pressure, back off from a storage rate limit, or fix clock skew between nodes).
The individual storage outcomes are already logged inside
tryExpireCurrentLock. Whatis missing is any way to attribute the thrown exception to a cause, and any signal at all
on two of the three paths.
This matters because a failed release leaves the lock file in storage without its
expired: trueflag — i.e. a dangling lock that blocks every other writer on that tableuntil the lease elapses.
Summary and Changelog
Each
FAILED_TO_RELEASEthrow now names its cause via a newgenerateLockStateMessage(LockState, String cause)overload, and each logs the contextneeded to act on it.
HEARTBEAT_STOP_FAILEDinterruptedflag to separate the two sub-cases inLockProviderHeartbeatManager#stopHeartbeat.INTERRUPTED_DURING_THROTTLE_BACKOFFInterruptedExceptionso the stack trace survives.THROTTLE_RETRIES_EXHAUSTEDEXPIRE_WRITE_FAILEDUNKNOWN_ERROR/ACQUIRED_BY_OTHERSoutcome.Additionally, on
ACQUIRED_BY_OTHERSwe now log how long ago our lease should have ended:Those two are indistinguishable today and need different fixes. Every new message also
carries
lockFilePath, so a lock left dangling in storage can be joined back to thewriter that failed to release it.
Detailed changes:
StorageBasedLockProvider: added thegenerateLockStateMessage(state, cause)overload; added alogger.errorat each of the three throw sites with the cause-specific context; passedieinto the interrupted-path log so the stack trace is retained; added the lease-overrun delta log onACQUIRED_BY_OTHERS.TestStorageBasedLockProvider: addedtestUnlockThrowsExceptionWhenInterruptedDuringThrottleBackoff(this path previously had no coverage); tightened the three existingFAILED_TO_RELEASEassertions to also pin the specific cause label, so a future refactor that collapses them fails the build.No code was copied.
Impact
None on behaviour. Control flow is untouched — every edit either adds a
logger.errorcall or appends
, cause <LABEL>to an existing exception message. No public API, config,or metric change (
updateLockReleaseFailureMetricdeliberately remains a single counter;splitting it per cause would change the metrics surface and is left for a separate
change).
Anything parsing these exception strings verbatim would see the appended
, cause <LABEL>suffix. The existingFAILED_TO_RELEASEsubstring is preserved.Risk Level
none — logging and exception-message text only.
Documentation Update
none — no new configs, no user-facing feature change.
Contributor's checklist
Verified locally on JDK 17:
TestStorageBasedLockProvider— 48 tests, 0 failures, 0 errors.