SoulKyu opened a new pull request, #3860: URL: https://github.com/apache/celeborn/pull/3860
### What changes were proposed in this pull request? In `StorageManager`: 1. `cleanupExpiredShuffleKey` now removes the expired shuffle key from `committedFileInfos` unconditionally. Previously the removal only ran when `workerGracefulShutdown` was enabled, and only if the key was also present in `diskFileInfos`. Only the DB entry deletion (`db.delete(...)`) remains conditional on graceful shutdown and `cleanDB`. 2. `notifyFileInfoCommitted` only records the committed file into `committedFileInfos` when `workerGracefulShutdown` is enabled, restoring the 0.5.x behavior. This map is only used to persist committed file infos to the recovery DB, so it is useless when graceful shutdown is disabled. ### Why are the changes needed? With the default configuration (`celeborn.worker.graceful.shutdown.enabled=false`), every committed `DiskFileInfo` is kept forever in `committedFileInfos`: - `TierWriter.notifyFileCommitted()` (called on every writer `close()`, for both local disk and DFS writers) calls `StorageManager.notifyFileInfoCommitted`, which adds the file info unconditionally. - `cleanupExpiredShuffleKey` only removes the entry under `if (workerGracefulShutdown)`, so the entries are never released. This is a regression introduced in 0.6: in 0.5.x, `PartitionDataWriter` only called `notifyFileInfoCommitted` under `if (workerGracefulShutdown)`. That guard was lost when the write path was refactored into `TierWriter`. The issue affects 0.6.1, 0.6.2 and `main`. Impact: the worker Old Gen grows without bound until `-Xmx`, leading to long Full GCs and eventually OOM. On a production 0.6.1 worker we observed ~14.25 million `DiskFileInfo` instances (and as many `ReduceFileMeta`) retained in the heap, while the disks only held ~6,900 shuffle files and the worker had only 176 active shuffles. Suggest backporting to `branch-0.6`. ### Does this PR resolve a correctness bug? <!-- Check if yes. The `correctness` label will be added/removed automatically. --> - [ ] Yes ### Does this PR introduce _any_ user-facing change? <!-- Check if yes. --> - [ ] Yes ### How was this patch tested? Added a unit test in `StorageManagerSuite`: with graceful shutdown disabled, notify a committed file for a shuffle key, expire that key via `cleanupExpiredShuffleKey`, and assert that `committedFileInfos` is empty. Existing tests are covered by CI. -- 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]
