YARN-11698. Store pending log aggregators in the NM State Store #6845
Conversation
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
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Kimahriman
force-pushed
the
nm-log-agg-state-store
branch
from
c497647 to
79e7bd9
Compare
|
🎊 +1 overall
This message was automatically generated. |
Kimahriman
force-pushed
the
nm-log-agg-state-store
branch
from
79e7bd9 to
6902a32
Compare
|
💔 -1 overall
This message was automatically generated. |
Kimahriman
force-pushed
the
nm-log-agg-state-store
branch
from
6902a32 to
652a7b6
Compare
|
🎊 +1 overall
This message was automatically generated. |
Kimahriman
force-pushed
the
nm-log-agg-state-store
branch
from
652a7b6 to
08cc91b
Compare
|
🎊 +1 overall
This message was automatically generated. |
zeekling
suggested changes
Sep 21, 2025
| if (!context.getContainers().containsKey(cid)) { | ||
| ApplicationId appId = | ||
| cid.getApplicationAttemptId().getApplicationId(); | ||
| if (isApplicationStopped(appId)) { |
There was a problem hiding this comment.
The current code will also delete all delegation tokens in the application, including those used for log aggregation, causing log aggregation to fail.
There was a problem hiding this comment.
Can you elaborate? Are you saying there is a problem with these changes? Or something else wrong in the existing code?
There was a problem hiding this comment.
I carefully reviewed the code again and there should be no problem of aggregation failure.
By the way.
Aggregate logs of containers that completed more than 30 minutes (or longer) to onto HDFS in advance, rather than waiting for the job to completed.
Is this implementation better than the current one?
There was a problem hiding this comment.
This doesn't actually change anything with how log aggregation works. It just prevents the node manager from holding onto finished containers in the store after the logs have been aggregated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Description of PR
Stores containers pending log aggregation in the NodeManager state store so logs can still be aggregated for complete containers after a Node Manager restart. This undoes and replaces https://issues.apache.org/jira/browse/YARN-4771 with a finer-grained approach that doesn't involve storing containers indefinitely until the application finishes.
The original approach has several issues, some of which were mentioned in the JIRA but decided it was ok:
Instead, this adds a new state store entry for containers pending log aggregation. This solves all the above issues, while still providing the same guarantees about logs being aggregated after a Node Manager restart.
How was this patch tested?
New UTs added
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?