YARN-11442. Refactor FederationInterceptorREST Code. #5420
Merged
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. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
goiri
approved these changes
Mar 14, 2023
| // Create a duplicate hsr. | ||
| final HttpServletRequest hsrCopy = clone(hsr); | ||
|
|
||
|
|
Contributor
Author
There was a problem hiding this comment.
Thank you for your help to review the code, I will modify the code. This part of the code, I need to continue to improve.
|
💔 -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. |
goiri
approved these changes
Mar 20, 2023
| * an exception will be thrown (No active SubCluster available to submit the request.) | ||
| */ | ||
| public static SubClusterId getRandomActiveSubCluster( | ||
| public SubClusterId getRandomActiveSubCluster( |
Member
There was a problem hiding this comment.
We should change the places where we call it to call it as static.
Contributor
Author
There was a problem hiding this comment.
Thank you very much for your help to review the code! This part of the code can continue to be static, I will modify the code.
| throws YarnException, IOException, InterruptedException { | ||
| throws YarnException { | ||
| SubClusterId subClusterId = | ||
| federationFacade.getRandomActiveSubCluster(subClustersActive, blackList); |
| ApplicationId applicationId = null; | ||
| try { | ||
| applicationId = ApplicationId.fromString(appId); | ||
| ApplicationId.fromString(appId); |
Member
There was a problem hiding this comment.
Why do we do this if we don't use it?
Contributor
Author
There was a problem hiding this comment.
Thanks for the reminder, I will improve this part of the code again.
| getOrCreateInterceptorForSubCluster(subClusterId, | ||
| subClusterInfo.getRMWebServiceAddress()); | ||
| // Call the appState interface. | ||
| DefaultRequestInterceptorREST interceptor = getOrCreateInterceptorForSubCluster(subClusterInfo); |
Member
There was a problem hiding this comment.
As you are consolidating, you could create methods with appId and nodeId.
|
|
||
| try { | ||
| long startTime = clock.getTime(); | ||
| SubClusterInfo subClusterInfo = getHomeSubClusterInfoByAppId(appId); |
Member
There was a problem hiding this comment.
Create one with appId and then you do the proper check for null, etc.
Contributor
Author
There was a problem hiding this comment.
I will modify the code.
| * application does not exist in StateStore. | ||
| */ | ||
| @Test | ||
| public void testGetApplicationStateNotExists() |
| Assert.assertNotNull(entity); | ||
| Assert.assertNotNull(entity instanceof ReservationListInfo); | ||
|
|
||
| assert entity instanceof ReservationListInfo; |
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Mar 21, 2023
|
|
||
| numSubmitRetries = conf.getInt( | ||
| YarnConfiguration.ROUTER_CLIENTRM_SUBMIT_RETRY, | ||
| numSubmitRetries = conf.getInt(YarnConfiguration.ROUTER_CLIENTRM_SUBMIT_RETRY, |
Member
There was a problem hiding this comment.
Avoid this change. It looked ok before.
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Mar 27, 2023
|
|
||
| protected DefaultRequestInterceptorREST getOrCreateInterceptorByAppId(String appId) | ||
| throws YarnException { | ||
| SubClusterInfo subClusterInfo = getHomeSubClusterInfoByAppId(appId); |
Member
There was a problem hiding this comment.
Why not doing the validation of the appId here?
Contributor
Author
There was a problem hiding this comment.
Thanks for your suggestion, I will modify the code.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@goiri Thank you very much for helping to review the code! |
ferdelyi
pushed a commit
to ferdelyi/hadoop
that referenced
this pull request
May 26, 2023
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.
JIRA: YARN-11442. Refactor FederationInterceptorREST Code.
Fix code issues to make code more readable