YARN-11357. Fix FederationClientInterceptor#submitApplication Can't Update SubClusterId #5055
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
…pdate SubClusterId.
|
💔 -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. |
4 tasks
cnauroth
approved these changes
Oct 22, 2022
Contributor
cnauroth
left a comment
cnauroth
left a comment
There was a problem hiding this comment.
+1 (binding). However, I would like to see one other code review from someone who has spent more time working in the YARN federation code.
@slfan1989 , thank you for the contribution.
Contributor
Author
|
@goiri Can you help to review the code? Thank you very much! In YARN-11342 (#5005), there is a problem with writing a judgment condition, which will cause the subClusterId to fail to update when retrying. The modified content is as follows: |
Contributor
Author
|
@goiri Can you help review this pr? Thank you very much! |
goiri
reviewed
Oct 24, 2022
...java/org/apache/hadoop/yarn/server/router/clientrm/TestFederationClientInterceptorRetry.java
Show resolved
Hide resolved
...java/org/apache/hadoop/yarn/server/router/clientrm/TestSequentialBroadcastPolicyManager.java
Show resolved
Hide resolved
| * Return to badCluster first. | ||
| */ | ||
| List<SubClusterId> subClusterIds = new ArrayList<>(preSelectSubClusters.keySet()); | ||
| if(subClusterIds.size() > 1){ |
...src/main/java/org/apache/hadoop/yarn/server/router/clientrm/FederationClientInterceptor.java
Show resolved
Hide resolved
Contributor
Author
|
@goiri Can you help review this pr again? Thank you very much! |
goiri
approved these changes
Oct 24, 2022
| LOG.info("Test submitApplication with two bad, one good SC."); | ||
|
|
||
| // This test must require the TestSequentialRouterPolicy policy | ||
| if (StringUtils.equals(routerPolicyManagerName, |
Member
There was a problem hiding this comment.
It may make sense to use something like:
Assume.assumeThat()
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. |
goiri
approved these changes
Oct 25, 2022
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@goiri Can you help to merge this pr into trunk branch? Thank you very much! |
Contributor
Author
|
@goiri 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
…pdate SubClusterId (apache#5055)
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-11357. Fix FederationClientInterceptor#submitApplication Can't Update SubClusterId.
The reason for the test failure:
In YARN-11342 (#5005), I refactored the getNewApplication, submitApplication method, but in the process of implementation, the judgment condition of if was written incorrectly.
If subCluster does not exist, we should add subCluster, if subCluster exists, we should update subCluster.
Q1：Why is the unit test for YARN-11342 passing?
In this test report(#4982 TestReport).
I found that
TestFederationClientInterceptorRetry#testSubmitApplicationOneBadOneGoodwas abnormal.In this test(
testSubmitApplicationOneBadOneGood), we should get good subCluster, but it returns bad SubCluster.When I tested it locally, I found that this unit test sometimes succeeded and sometimes failed.
This is related to the
UniformBroadcastPolicyManager,UniformRandomRouterPolicyis used, this policy will randomlySelect 1 SubCluster from Active SubClusters.
If we get the good subCluster for the first time, then this test can be executed successfully, but if we get the bad subCluster for the first time, then we will retry 3 times, because the judgment of
FederationClientInterceptor#submitApplicationis wrong, As a result, the good subcluster cannot be updated to the stateStore. So We get the wrong subcluster.Q2: How can we verify that this part of the code is accurate after modification.
For unit testing, I redesigned a new RouterPolicy#TestSequentialRouterPolicy, which can return SubClusterId in order.
Example:
We have 3 subClusters, 1 goodSubCluster and 2 badSubClusters.
The scId of goodSubCluster is 0, and the scId of badSubCluster is 1 and 2. We hope Return to badSubCluster first.
We want to return the badSubCluster first, we can sort the subClusterId in reverse order, so we can get [2, 1, 0], which is [bad2,bad1,good]
Test case 1:
We set the number of retries to 1, and then
submitApplicationwill be executed 2 times.The 1st ,
bad2will be selected, The 2nd,bad1will be selected.If we query the StateStore we should get
bad1.Test case 2:
We set the number of retries to 2, and then
submitApplicationwill be executed 3 times.The 1st ,
bad2will be selected, The 2nd,bad1will be selected, The 3rd good cluster will be selected.If we query the StateStore we should get
good.