YARN-11856. DOWNLOADING resources unlock and cleanup is interrupted when killing a container that is localizing - #8691
Open
eubnara wants to merge 2 commits into
Open
YARN-11856. DOWNLOADING resources unlock and cleanup is interrupted when killing a container that is localizing#8691eubnara wants to merge 2 commits into
eubnara wants to merge 2 commits into
Conversation
…hen killing a container that is localizing. Contributed by zheng-weihao and Yubi Lee.
4 tasks
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes NodeManager localization cleanup when a container is killed during localization and event dispatch fails due to interruption, ensuring resources aren’t left stuck in DOWNLOADING and cleanup work still runs.
Changes:
- Wrap
ContainerResourceFailedEventdispatch inLocalizerRunner.run()with atry/catchso cleanup logic always executes. - Add a unit test that simulates a dispatch failure (
YarnRuntimeException(InterruptedException)) and asserts resources are unlocked and deletions are scheduled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.../ResourceLocalizationService.java |
Ensures cleanup runs even if dispatching ContainerResourceFailedEvent throws during interruption. |
.../TestResourceLocalizationService.java |
Adds regression test covering dispatch-failure cleanup behavior for DOWNLOADING resources. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…evel logging, stronger test assertions. - Restore the thread interrupt status when the swallowed dispatch failure was caused by an InterruptedException. - Log the dispatch failure at WARN with parameterized logging. - Assert the exact FileDeletionTasks (localization dir + _tmp dir, nmPrivate token file) and the restored interrupt status in the test.
Contributor
Author
|
Addressed the Copilot review comments in 8600dbb: restored the interrupt status when the swallowed failure was caused by an |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
@slfan1989 @cnauroth Could you take a look when you have a chance? This continues #7893 (YARN-11856): when a container is killed while localizing, dispatching ContainerResourceFailedEvent can throw (InterruptedException from the dispatcher), which escapes LocalizerRunner before the DOWNLOADING resources are unlocked/cleaned up, leaving other containers waiting on the same private resource stuck in LOCALIZING. This PR catches the dispatch failure (restoring the interrupt flag) so the cleanup always runs, plus a test reproducing the kill-during-localization case. Thanks! |
slfan1989
approved these changes
Aug 30, 2026
slfan1989
left a comment
slfan1989
left a comment
Contributor
There was a problem hiding this comment.
Thanks for addressing the previous review comments.
I reviewed the latest revision and the fix looks good to me. Catching the dispatch failure ensures that the remaining cleanup always runs, while restoring the interrupt status preserves the LocalizerRunner thread's interrupt semantics.
The updated test now verifies the exact deletion tasks for the localization directory, the _tmp directory, and the nmPrivate token file, as well as the restored interrupt status.
The current ASF license failure is unrelated to this PR and comes from the vendored JSON.java file in hadoop-hdfs-rbf.
No blocking issues from my side. +1.
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
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.
Description of PR
When a container is killed while it is localizing, the
LocalizerRunnerthread is interrupted. In the
finallyblock ofLocalizerRunner.run(),dispatching the
ContainerResourceFailedEventthen throwsYarnRuntimeException(InterruptedException), which skips everything afterit: resources left in DOWNLOADING state are never unlocked, and the
deletion tasks for the localization dirs, the
_tmpdownload dirs and thenmPrivate token file are never scheduled.
This PR revives the fix from #7893 by @zheng-weihao (stale-closed after
100 days of inactivity), unchanged: the event dispatch is wrapped in
try/catch so the cleanup below it always runs. The remaining review
comment on #7893 (dropping
FSErrorfrom the catch list) had already beenaddressed in its final revision. Credit to the original author is kept in
the commit message.
What this PR adds on top of #7893 is a unit test, which was the other
blocker ("no new or modified tests").
We hit this in production on a ~600 NodeManager cluster running
DefaultContainerExecutor: because DCE runs the localizer inside the NM
JVM, every kill-during-localization also leaked DFS block reader sockets
into the long-lived NM process, leaving DataNodes with FIN_WAIT1
connections whose send queues never drain. The HDFS side of that leak is
tracked separately in HDFS-17965 (#8690); this issue is the YARN-side
trigger and also leaves resources stuck in DOWNLOADING state regardless of
the container executor in use.
How was this patch tested?
New unit test
TestResourceLocalizationService#testDownloadingResourcesCleanedUpWhenDispatchFails:a mocked dispatcher throws
YarnRuntimeException(InterruptedException)when the
ContainerResourceFailedEventis dispatched, simulating thekill-during-localization interrupt. Without the fix,
run()propagatesthe exception and the DOWNLOADING resource is never unlocked (test fails).
With the fix, the resource is unlocked and the deletion tasks are
scheduled (test passes).
For code changes:
declared according to the connector-specific documentation? Note: Automated CI
testing doesn't cover all cases so manual testing with cloud storage is still
required.
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
If an AI tool was used:
where is the name of the AI tool used.
Contains content generated by Claude Code (Anthropic Claude): the
new unit test. The fix itself is unchanged from YARN-11856. DOWNLOADING resources unlock and cleanup is interrupted w… #7893. All content
was human-reviewed and complies with the ASF Generative Tooling
Guidance (https://www.apache.org/legal/generative-tooling.html).
https://www.apache.org/legal/generative-tooling.html