YARN-11274. Impove Nodemanager#NodeStatusUpdaterImpl Log.#4783
Merged
ayushtkn merged 4 commits intoapache:trunkfrom Sep 10, 2022
Merged
YARN-11274. Impove Nodemanager#NodeStatusUpdaterImpl Log.#4783ayushtkn merged 4 commits intoapache:trunkfrom
ayushtkn merged 4 commits intoapache:trunkfrom
Conversation
slfan1989
commented
Aug 22, 2022
| runningApplications.add(appEntry.getKey()); | ||
| } | ||
| } | ||
| LOG.info("Running Applications Size : {}.", runningApplications.size()); |
Contributor
Author
There was a problem hiding this comment.
add log, print the number of running apps.
slfan1989
commented
Aug 22, 2022
|
|
||
| if (containerReports != null && !containerReports.isEmpty()) { | ||
| LOG.info("Registering with RM using containers :" + containerReports); | ||
| LOG.info("Registering with RM using containers.size : {}." + containerReports.size()); |
Contributor
Author
There was a problem hiding this comment.
It is enough to print the number directly.
This function will call getNMContainerStatuses()
Line#394: getNMContainerStatuses()
The list of Containers has been printed, no need to reprint.
Line#402: I modified it to print size directly.
|
🎊 +1 overall
This message was automatically generated. |
Samrat002
reviewed
Aug 22, 2022
Contributor
There was a problem hiding this comment.
@slfan1989
in code changes , there are changes where inplace string concatination is replaced with formated replacement using {}. For example
LOG.info("Sending out " + containerStatuses.size()
+ " NM container statuses: " + containerStatuses);
is changed with
LOG.info("Sending out {} container NM container statuses: {}.",
containerStatuses.size(), containerStatuses);
any specific reason to make this change ?
| + " hence resyncing."); | ||
| LOG.warn("Message from ResourceManager: " | ||
| + response.getDiagnosticsMessage()); | ||
| LOG.warn("Node is out of sync with ResourceManager, hence resyncing."); |
Contributor
There was a problem hiding this comment.
Both can be combined into 1 LOG.warn
Contributor
Author
There was a problem hiding this comment.
Thanks for your suggestion, I will modify the code.
Contributor
Author
|
@Samrat002 The purpose of this pr is to increase the printing of the number of running apps and avoid the repeated printing of container information. From my personal point of view, I think it is better to use placeholders for printing logs, so I modified it. Thanks again for your suggestion. |
Contributor
Author
|
@ayushtkn Can you help review this pr? Thank you very much! |
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
reviewed
Sep 9, 2022
Member
ayushtkn
left a comment
ayushtkn
left a comment
There was a problem hiding this comment.
Minor comments, overall looks good
| public NodeHeartbeatResponse nodeHeartbeat(NodeHeartbeatRequest request) | ||
| throws YarnException, IOException { | ||
| LOG.info("Got heartBeatId: [" + heartBeatID +"]"); | ||
| LOG.info("Got heartBeatId: [{}}]", heartBeatID); |
Member
There was a problem hiding this comment.
there are two }}, is that intentional?
Contributor
Author
There was a problem hiding this comment.
Thanks for your help reviewing the code. I will modify the code, the last } is redundant.
Comment on lines
1133
to
1134
| LOG.error("NM node attributes {{}} were not accepted by RM and message from RM : {}." , | ||
| getPreviousValue(), response.getDiagnosticsMessage() ); |
Member
There was a problem hiding this comment.
I am not sure about this {{}}, give a check if it works they way you want, in general it is very confusing can explore using [{}] as well
Contributor
Author
There was a problem hiding this comment.
I agree with you, I will modify the code.
Comment on lines
1290
to
1284
| LOG.error( | ||
| "NM node labels {" + StringUtils.join(",", getPreviousValue()) | ||
| + "} were not accepted by RM and message from RM : " | ||
| + response.getDiagnosticsMessage()); | ||
| LOG.error("NM node labels {{}} were not accepted by RM and message from RM : {}.", | ||
| StringUtils.join(",", getPreviousValue()), response.getDiagnosticsMessage()); |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@ayushtkn Thank you very much for helping to review the code! |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…. Contributed by fanshilun. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org>
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
4 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.
JIRA:YARN-11274. Impove Nodemanager#NodeStatusUpdaterImpl Log.
The improvements are as follows:
1.When RM failover, NM will re-register, report the list of running apps, But there is no log, add this part of the log, print and report the number of apps.
2.registerWithRM#containerReports is enough to print the number of prints, because the above calling process has already printed the container list, so there is no need to repeat printing.
3.Use placeholders to display logs to avoid string concatenation.