-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
YARN-11531. [Federation] Code cleanup for NodeManager#amrmproxy. #5841
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -53,7 +53,7 @@ public class AMRMProxyApplicationContextImpl implements | |||
* @param nmContext NM context | |||
* @param conf configuration | |||
* @param applicationAttemptId attempt id | |||
* @param user user name of the application | |||
* @param user username of the application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave this as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for reviewing the code! I will fix it.
subClusterId.getId(), amRegistrationRequest); | ||
|
||
// Set sub-cluster to be timed out initially | ||
lastSCResponseTime.put(subClusterId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix it.
@@ -268,7 +267,7 @@ private void releaseContainersAndAssert(List<Container> containers) | |||
// aggregated and returned back to us, and we can check if total request size | |||
// and returned size are the same | |||
List<ContainerId> containersForReleasedContainerIds = | |||
new ArrayList<ContainerId>(); | |||
new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation looks funny.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix it.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri Can you help to review this PR again? Thank you very much! |
@goiri Thank you very much for your help in reviewing the code! |
Description of PR
JIRA: YARN-11531. [Federation] Code cleanup for NodeManager#amrmproxy.
The main improvements are as follows:
How was this patch tested?
This pr does not change the code logic and does not require additional unit tests.
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?