Repository navigation
YARN-11993. PublicLocalizer thread exits permanently when pending.remove() returns null - #8755
Merged
Merged
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. |
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
3 of 6 tasks
gerashegalov
reviewed
Oct 1, 2026
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
This was referenced Oct 4, 2026
Merged
…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>
The latch in testPublicLocalizerSurvivesUnknownResource counted down inside the callable, before the FutureTask completed and was queued, so the test only showed that nothing broke within 5s, not that the localizer had taken the unknown resource and carried on. Waiting for the "Localized unknown resource" log alone is not enough either: run() wrote that line before the old return too. The test now waits for the log, then submits a second unknown resource and waits for that to be logged as well, which only a localizer still taking from the queue can do. On the unfixed code the second submit is rejected because the pool has shut down. The test also no longer spends a fixed 5s waiting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VAqFrhFHxmwcfKkxfYf8Df
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@gerashegalov It is ready to be merged, yetus just gave the +1. |
Contributor
|
@joseluisll it is LGTM. Thank you for this fix. I have not committed to Apache Hadoop recently. I would like an active committer to take a look and merge it. @slfan1989 @ayushtkn could you take a look? |
slfan1989
approved these changes
Oct 7, 2026
slfan1989
left a comment
slfan1989
left a comment
Contributor
There was a problem hiding this comment.
LGTM.
The change correctly keeps the PublicLocalizer running when a completed download is missing from pending.
I verified that the new regression test passes with the fix and fails when the original return is restored.
The latest Yetus run and GitHub Actions Build are green. Ready to merge.
Contributor
|
@joseluisll Thanks for the contribution! Merged into trunk. @gerashegalov @ayushtkn Thanks for the review! |
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
6 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 edits TestRouterWebServicesREST.java:719-742, where TestFederationWebApp fails at line 737 in this PR's precommit, still red in the latest run
Required by (waits for this one to be merged):
Description of PR
YARN-11993. PublicLocalizer thread exits permanently when pending.remove() returns null.
When
PublicLocalizer.run()dequeues a completed download that is not inpending,it
returns. Thefinallyblock then shuts down the download pool, and every laterpublic localization on the node is rejected until the NodeManager restarts.
This PR changes that
returntocontinue, so the localizer logs the unknownentry and keeps running.
YARN-11998 moved this null check for SpotBugs but kept the
return, so the bugis still on trunk.
Also removes a stale SpotBugs exclusion for
PublicLocalizer.work, a method thatno longer exists.
How was this patch tested?
Added
TestResourceLocalizationService#testPublicLocalizerSurvivesUnknownResource.It fails on current trunk with
RejectedExecutionException(pool terminated) andpasses with this change. All 19 tests in
TestResourceLocalizationServicepass.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?🤖 Generated with Claude Code