-
Notifications
You must be signed in to change notification settings - Fork 29.4k
[SPARK-59439][CORE] Release the HistoryServerDiskManager lease reservation exactly once on a failed commit - #58741
Conversation
...ation exactly once on a failed commit Lease.commit() releases the reservation before the fallible rename; when the rename fails and the caller rolls back, the reservation is released a second time, driving the usage tracker negative. Funnel both commit() and rollback() through an idempotent releaseLease() so it is returned exactly once.
@uros-b
uros-b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongjoon-hyun
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix. The change looks correct to me: the reservation is now returned exactly once across all failure paths in commit() (before and after releaseLease()), and it also covers FsHistoryProvider.createHybridStore(), where onSwitchToDiskStoreFail calls lease.rollback() after a failed (or even a successful) commit() on the background thread.
A minor thing in the PR description: could you record the actual model name and version in Generated-by (e.g., Generated-by: Claude Opus 4.8) instead of just Claude?
Uh oh!
There was an error while loading. Please reload this page.
What changes were proposed in this pull request?
HistoryServerDiskManager.Leasenow releases its reserved (uncommitted) usage exactly once, whether the lease is committed, fails to commit, or is rolled back. The twoupdateUsage(-leased)calls incommit()androllback()are replaced by an idempotentreleaseLease()helper guarded by areleasedflag.A regression test forces the rename in
commit()to fail, then callsrollback()(as the caller does) and asserts the usage tracker returns to zero rather than going negative, and that a subsequent lease/commit still succeeds.Why are the changes needed?
Lease.commit()releases the reservation withupdateUsage(-leased)before renaming the temporary store into place. SPARK-58985 made that rename throw anIOExceptionon failure, which happens after the reservation has already been released. The caller then rolls the lease back -- e.g.FsHistoryProvider.createDiskStore()callslease.rollback()onIOException-- androllback()releases the reservation a second time.The reservation is added once (in
lease()) but subtracted twice, so the current-usage tracker is under-counted by the leased amount and can go negative, throwing:This is the same crash SPARK-58985 aimed to prevent; it is reachable whenever
renameTofails (I/O error, full disk, destination parent removed out of band). IncreateDiskStore()'s retry loop the exception also escapes the loop, so the store is never rebuilt.Does this PR introduce any user-facing change?
No.
How was this patch tested?
New unit test in
HistoryServerDiskManagerSuite(runs for both the LevelDB and RocksDB backends). It fails on the current code withIllegalStateException: Disk usage tracker went negativeand passes with this change.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.8