YARN-11993. PublicLocalizer thread exits permanently when pending.remove() returns null - #8755
Open
joseluisll wants to merge 1 commit into
Open
joseluisll wants to merge 1 commit into
joseluisll wants to merge 1 commit into
Conversation
|
🎊 +1 overall
This message was automatically generated. |
joseluisll
force-pushed
the
YARN-11993
branch
from
September 25, 2026 10:45
2f5e6e5 to
d463bd3
Compare
joseluisll
changed the title
|
💔 -1 overall
This message was automatically generated. |
…ove() returns null When pending.remove(completed) returned null, run() logged "Localized unknown resource" and returned. That terminated the Public Localizer thread for the life of the NodeManager, and run()'s finally block shut the download pool down on the way out. Every later public-resource request was then rejected; YARN-1800's handler turns each RejectedExecutionException into a ResourceFailedLocalizationEvent, so containers fail rather than hang, but no public resource can be localized on that node again until the NM is restarted. That is the state reported in YARN-9968, which fixed a different branch of this method. Skip the unattributable resource with continue instead. The branch is defensive rather than reachable today: addResource() wraps queue.submit(...) and pending.put(...) in one synchronized (pending) block so a future cannot complete and be dequeued before the map is updated, and pending is a synchronizedMap on the same monitor. It matters because YARN-11746 proposes shutting the NodeManager down when this thread exits, which would turn a single unknown resource into a node-wide outage. Hoisting the null check out of the try block also makes the assoc dereference in the ExecutionException handler provably safe, so SpotBugs no longer reports NP_NULL_ON_SOME_PATH_EXCEPTION. The exclude entry for it is therefore dropped rather than repaired; it had been inert since the revert of HADOOP-19668/19670 (apache#8568) restored run() without restoring the method name YARN-11912 changed to work(). The new test submits a download straight to the completion queue so the Future is never recorded in pending, then asserts the localizer neither dies nor shuts its pool down. It fails on the unfixed code with "Public Localizer exited after taking an unknown resource". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
joseluisll
force-pushed
the
YARN-11993
branch
from
September 25, 2026 17:22
d463bd3 to
4c8d772
Compare
joseluisll
changed the title
|
💔 -1 overall
This message was automatically generated. |
This was referenced Sep 27, 2026
This branch has not been deployed
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
2 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.
Depends on (has to be merged first):
clears a CI failure: it fixes TestLogAggregationService, which fails in this PR's precommit, still red in the latest run
mutual: each PR clears a CI failure of the other, so neither goes green alone - merge them back to back
Required by (waits for this one to be merged):
mutual: each PR clears a CI failure of the other, so neither goes green alone - merge them back to back
Description of PR
In
ResourceLocalizationService$PublicLocalizer.run(), whenpending.remove(completed)returns null the loop logs"Localized unknown resource"and returns, terminating the Public Localizer thread for the life of the NodeManager.run()'sfinallyblock then shuts the download pool down, so every later public-resource request is rejected — YARN-1800's handler fails each one rather than hanging it, but no public resource can be localized on that node again until the NM restarts. That is the state reported in YARN-9968, which fixed a different branch of this method.Expected: an unattributable localized resource is skipped and the localizer keeps serving later requests. This PR uses
continue, hoisted out of thetry.The branch is defensive rather than reachable today —
addResource()putsqueue.submit(...)andpending.put(...)in onesynchronized (pending)block for exactly this reason. It matters because YARN-11746 proposes shutting the NodeManager down when this thread exits, which would turn one unknown resource into a node-wide outage.Hoisting the check also makes the
assocdereference in theExecutionExceptionhandler provably safe, clearingNP_NULL_ON_SOME_PATH_EXCEPTION— the only extant SpotBugs warning in this module, which currently costs every NodeManager PR a-1 spotbugs. The exclude entry is therefore deleted rather than repaired; it had been inert since #8568 restoredrun()without restoring the name YARN-11912 changed towork().Still leaked, out of scope: the
// TODO deletefile. Withoutassocthere is noLocalResourceRequestto fire a failure event for.How was this patch tested?
New test
TestResourceLocalizationService#testPublicLocalizerSurvivesUnknownResourcesubmits a download straight to the completion queue so theFutureis never recorded inpending, then asserts the localizer neither dies nor shuts its pool down. Against unmodified trunk it fails withPublic Localizer exited after taking an unknown resource; with the fix it passes.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?🤖 Generated with Claude Code