HDFS-16831. [RBF SBN] GetNamenodesForNameserviceId should shuffle Observer NameNodes every time #5098
Conversation
This file contains 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
…erver NameNodes every time
|
💔 -1 overall
This message was automatically generated. |
|
@omalley @simbadzina Master, please help me review this PR too. The failed building may not relate this change. It works well locally. The method |
goiri
reviewed
Nov 2, 2022
| List<T> inputNameNodes, boolean listObserversFirst) { | ||
| if (!listObserversFirst) { | ||
| return inputNameNodes; | ||
| } else { |
There was a problem hiding this comment.
No ned for elses when we do the return.
| } | ||
| } | ||
|
|
||
| if (observerNNList.size() <= 1) { |
There was a problem hiding this comment.
We could've done this check earlier.
|
@goiri Sir, thanks for your review. I have updated this PR, please help me review it again when you are available. Thanks so much. |
|
💔 -1 overall
This message was automatically generated. |
|
@ZanderXu Thanks for your contribution, we should fix the checkstyle issue. |
@slfan1989 Sir, thanks for your review. I have fixed the checkstyle in the last commit. |
|
💔 -1 overall
This message was automatically generated. |
ashutoshcipher
approved these changes
Nov 7, 2022
There was a problem hiding this comment.
+1 LGTM after last commit. @ZanderXu - Do you think we need to add a UT ?
@ashutoshcipher Sir, thanks for your review. I will add one UT to test it. Do you have some good ideas to test shuffling result? |
|
@ashutoshcipher @goiri @slfan1989 Sir, I have add one UT to test it. Please help me review it again. Thanks so much. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Nov 8, 2022
.../main/java/org/apache/hadoop/hdfs/server/federation/resolver/MembershipNamenodeResolver.java
Show resolved
Hide resolved
.../main/java/org/apache/hadoop/hdfs/server/federation/resolver/MembershipNamenodeResolver.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Sir, can help me review this PR again? Thanks so much. |
|
@simbadzina Master, can you help me review this PR when you are available? |
Code looks good to me. Just one small recommended change to test the shuffling. Thanks for working on this. |
simbadzina
requested changes
Dec 21, 2022
There was a problem hiding this comment.
To test the shuffling, one approach would be to call namenodeResolver.getNamenodesForNameserviceId() 100 times in a loop and check if ever the two observers swap positions in the list. If there is never a swap we fail the test.
The test will be probabilistic but the chance of it flaking will be extremely low 0.5^100.
List<? extends FederationNamenodeContext> observerList3 ;
for(int i = 0; i < 100; i++) {
observerList3 = namenodeResolver.getNamenodesForNameserviceId(NAMESERVICES[0], true);
assertEquals(FederationNamenodeServiceState.OBSERVER, observerList3.get(0).getState());
assertEquals(FederationNamenodeServiceState.OBSERVER, observerList3.get(1).getState());
if (observerList3.get(0).getNamenodeId().equals(observerList2.get(1).getNamenodeId()) &&
observerList3.get(1).getNamenodeId().equals(observerList2.get(0).getNamenodeId())) {
return;
}
}
Assert.fail("Observer order never changed.");
|
\
@simbadzina Thanks for your nice suggestion, I will do it. |
goiri
approved these changes
Dec 22, 2022
|
@simbadzina master, I have modified the UT, please help me review again, thanks. |
|
🎊 +1 overall
This message was automatically generated. |
simbadzina
approved these changes
Dec 23, 2022
|
Merged, thanks @ashutoshcipher @simbadzina @slfan1989 @goiri for your review. Thanks so much. |
LiuGuH
pushed a commit
to LiuGuH/hadoop
that referenced
this pull request
Mar 29, 2024
…erver NameNodes every time (apache#5098)
LiuGuH
pushed a commit
to LiuGuH/hadoop
that referenced
this pull request
Jul 4, 2024
…erver NameNodes every time (apache#5098)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of PR
HDFS-16831
The method
getNamenodesForNameserviceIdinMembershipNamenodeResolver.classshould shuffle Observer NameNodes every time. The current logic will return the cached list and will caused all of read requests are forwarding to the first observer namenode.The related code as bellow: