pan3793 opened a new pull request, #58312:
URL: https://github.com/apache/spark/pull/58312
### What changes were proposed in this pull request?
Make the disk usage accounting in `HistoryServerDiskManager` atomic with the
store operation it accompanies:
- `release()` now performs the whole operation -- removing the app from the
`active` map, updating usage accounting, and deleting or re-measuring the store
-- under the `active` lock.
- `makeRoom()` re-checks each eviction candidate under the `active` lock
before deleting it, skipping candidates that became active or whose store
directory is already gone. The summary log reports the stores actually deleted
and the space actually freed.
The lock now covers directory I/O (`sizeOf`, deletion, listing read/write),
the same pattern `Lease.commit` already uses under this lock.
### Why are the changes needed?
`HistoryServerDiskManager` can deduct the same store size twice, driving the
committed usage negative and making the History Server throw
`IllegalStateException: Disk usage tracker went negative`.
The race is longstanding: `release()` updates usage and operates on the
store directory outside the `active` lock, and `makeRoom()` deletes eviction
candidates without re-checking, so two paths have been able to deduct the same
store since the disk manager was introduced by SPARK-20654 (2.3.0).
SPARK-56044 (4.0.3) widened the race. By adding a deduction in `release()`
based on the size measured from disk for apps not in the `active` map, a double
deduction no longer requires the application to be actively open:
- `release(delete = true)` vs `openStore()`: `release()` deducts the
measured size and deletes the store, but a concurrent `openStore()`
re-registered the app in `active`, so a subsequent `release()` deducts the size
again.
- `release(delete = true)` vs `makeRoom()`: both paths deduct the size of
the same store.
This makes the race reachable in normal History Server operation, e.g. log
cleanup calling `release(delete = true)` for an app never opened after a
restart while a concurrent UI request opens or evicts the same store. The fix
also closes two concurrent `makeRoom()` calls evicting the same store twice.
This crash was observed on a production History Server (4.1-based build), in
the periodic log cleanup path:
```
java.lang.IllegalStateException: Disk usage tracker went negative (now =
-118595158, delta = -151974353)
at
o.a.s.deploy.history.HistoryServerDiskManager.updateUsage(HistoryServerDiskManager.scala:285)
at
o.a.s.deploy.history.HistoryServerDiskManager.release(HistoryServerDiskManager.scala:186)
at
o.a.s.deploy.history.FsHistoryProvider.cleanAppData(FsHistoryProvider.scala:746)
at
o.a.s.deploy.history.FsHistoryProvider.deleteAttemptLogs(FsHistoryProvider.scala:1132)
at
o.a.s.deploy.history.FsHistoryProvider.cleanLogs(FsHistoryProvider.scala:1063)
at
o.a.s.deploy.history.FsHistoryProvider.$anonfun$startPolling$4(FsHistoryProvider.scala:305)
```
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added a new test `SPARK-58985: release with delete is atomic with openStore`
in `HistoryServerDiskManagerSuite`, which fails on the pre-fix code with
`IllegalStateException: Disk usage tracker went negative` and passes with the
fix.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Qwen 3.8 Max
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
