-
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-11180. Refactor some code of getNewApplication, submitApplication etc. #4618
Conversation
…n, forceKillApplication, getApplicationReport.
...src/main/java/org/apache/hadoop/yarn/server/router/clientrm/FederationClientInterceptor.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -364,7 +362,7 @@ public void testForceKillApplicationNotExists() | |||
Assert.fail(); | |||
} catch (YarnException e) { | |||
Assert.assertTrue(e.getMessage().equals( |
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.
assertEquals
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.
Thanks for helping to review the code, I will fix it.
@@ -364,7 +362,7 @@ public void testForceKillApplicationNotExists() | |||
Assert.fail(); |
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.
LambdaTestUtils#intercept
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.
long startTime = clock.getTime(); | ||
Map<SubClusterId, SubClusterInfo> subclusters = | ||
Map<SubClusterId, SubClusterInfo> subClusters = |
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.
try { | ||
clusterMetrics = invokeConcurrent(subclusters.keySet(), remoteMethod, | ||
clusterMetrics = invokeConcurrent(subClusters.keySet(), remoteMethod, |
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 fits?
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.
|
||
if (activeSubclusters == null || activeSubclusters.size() < 1) { | ||
Map<SubClusterId, SubClusterInfo> activeSubClusters) throws YarnException { | ||
if (activeSubClusters == null || activeSubClusters.size() < 1) { |
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.
isEmpty()?
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.
Your suggestion is right, I will modify the code.
@@ -280,47 +281,47 @@ private SubClusterId getRandomActiveSubCluster( | |||
public GetNewApplicationResponse getNewApplication( | |||
GetNewApplicationRequest request) throws YarnException, IOException { | |||
|
|||
long startTime = clock.getTime(); | |||
if(request == null) { |
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.
if (request == null) {
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. |
🎊 +1 overall
This message was automatically generated. |
@goiri Please help me review the code again, thank you very much. |
LGTM +1 |
|
||
if (activeSubclusters == null || activeSubclusters.size() < 1) { | ||
Map<SubClusterId, SubClusterInfo> activeSubClusters) throws YarnException { | ||
if (activeSubClusters.isEmpty()) { |
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.
Shouldn't we check for null?
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.
...src/main/java/org/apache/hadoop/yarn/server/router/clientrm/FederationClientInterceptor.java
Show resolved
Hide resolved
Map<SubClusterId, SubClusterInfo> subClustersActive = | ||
federationFacade.getSubClusters(true); | ||
|
||
GetNewApplicationResponse response = null; |
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.
Why not leave the initialization where it was?
RouterAuditLogger.logFailure(user.getShortUserName(), GET_NEW_APP, UNKNOWN, | ||
TARGET_CLIENT_RM_SERVICE, errMsg); | ||
RouterServerUtil.logAndThrowException(errMsg, null); | ||
return response; |
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.
return? Aren't we throwing the exception?
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.
Your suggestion is right, I will throw exception directly,thank you.
RouterAuditLogger.logFailure(user.getShortUserName(), SUBMIT_NEW_APP, UNKNOWN, | ||
TARGET_CLIENT_RM_SERVICE, msg, applicationId); | ||
RouterServerUtil.logAndThrowException(msg, null); | ||
return response; |
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.
This pattern is very weird.
There is no way to let know that we always throw the exception?
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 modify the code.
@@ -293,17 +288,15 @@ public void testSubmitApplicationEmptyRequest() | |||
interceptor.submitApplication(null); | |||
Assert.fail(); |
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.
LambdaTestUtils#intercep
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 for helping to review the code, I will fix it.
💔 -1 overall
This message was automatically generated. |
@goiri Please help me review the code again, thank you very much! |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri Please help me to review the code again, thank you very much! |
🎊 +1 overall
This message was automatically generated. |
@goiri Can you help me merge this pr into trunk branch? I want to finish YARN-11177 based on this, thank you very much! |
@goiri thank you very much! |
JIRA: YARN-11180. Refactor some code of getNewApplication, submitApplication, forceKillApplication, getApplicationReport.
1) FederationClientInterceptor#getNewApplication
1.Increase request is empty check
2.Use RouterServerUtil.logAndThrowException instead of throw YarnRuntime Exception.
2) FederationClientInterceptor#submitApplication/forceKillApplication/getApplicationReport/getApplications
1.Use RouterServerUtil.logAndThrowException instead of throw YarnRuntime Exception.
2.Use string.format instead of +
3.Fix Code Style.
3) FederationClientInterceptor#getClusterMetrics
1.Increase request is empty check
4) FederationClientInterceptor#getClusterNodes/getQueueUserAcls/listReservations/getNodeToLabels/getLabelsToNodes/getClusterNodeLabels
1.Use RouterServerUtil.logAndThrowException instead of throw YarnRuntime Exception.