viirya opened a new pull request, #59851:
URL: https://github.com/apache/spark/pull/59851
### What changes were proposed in this pull request?
This is a follow-up to 18f88fba9d0 ("[CORE][MINOR] Improve ESS file
consistency"), which added read-time block path verification to
`ExternalShuffleBlockResolver` (`spark.shuffle.service.verifyBlockPathsAtRead`,
default `true`).
- `applicationRemoved()` now evicts the removed executors' local dirs from
`canonicalLocalDirs`, keeping any dir that is still referenced by another
registered executor. `executorRemoved()` does not evict, matching how
`executors` is handled there: the shuffle service keeps serving a removed
executor's shuffle files until its application is removed.
- The index file path is now verified only on a `shuffleIndexCache` miss,
i.e. when the index file is actually read. The data file is still verified on
every fetch, since it is opened on every fetch.
- Document `spark.shuffle.service.verifyBlockPathsAtRead` in
`docs/configuration.md`.
### Why are the changes needed?
`canonicalLocalDirs` is filled in `registerExecutor()` and lazily in
`getVerifiedFile()`, but nothing ever removes entries. On YARN every executor's
local dirs are unique (`.../appcache/<appId>/blockmgr-<uuid>`), so a
long-running NodeManager-hosted shuffle service grows the map with every
executor it has ever served.
Also, each shuffle block fetch did two `getCanonicalFile()` calls (index and
data) before the index cache lookup, so the cache did not save the index path
canonicalization even when the index file was not read.
### Does this PR introduce _any_ user-facing change?
No behavior change other than fixing the unbounded memory growth in the
shuffle service. The config was previously undocumented and is now documented.
### How was this patch tested?
New tests in `ExternalShuffleBlockResolverSuite`:
- `canonicalLocalDirsEvictedOnApplicationRemoved`: registers 2 apps x 100
executors plus a dir shared by both apps, checks that `executorRemoved` keeps
entries, that removing one app keeps the shared dir, and that removing both
empties the map. It fails without this fix (201 entries left instead of 101).
- `testIndexFileResolvingOutsideLocalDirsRejected`: an index file that is a
symlink outside the registered local dirs is still rejected when it is loaded
into the index cache.
Ran `ExternalShuffleBlockResolverSuite`, `ExternalBlockHandlerSuite`,
`ExternalShuffleCleanupSuite` and `ExternalShuffleIntegrationSuite` on JDK 17.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
This pull request and its description were written by Isaac.
--
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]
