HBASE-25553 It is better for ReplicationTracker.getListOfRegionServer… #2928
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
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
approved these changes
Feb 5, 2021
There was a problem hiding this comment.
LGTM. Have just a small suggestion, fine to go without it, up to you @ddupg .
| @@ -168,7 +169,7 @@ private boolean refreshOtherRegionServersList(boolean watch) { | |||
| } else { | |||
| synchronized (otherRegionServers) { | |||
| otherRegionServers.clear(); | |||
| otherRegionServers.addAll(newRsList); | |||
| newRsList.stream().map(ServerName::parseServerName).forEach(otherRegionServers::add); | |||
There was a problem hiding this comment.
Nit: Maybe worth do this conversion inside getRegisteredRegionServers method, instead, as it's the first point of entry from ZK where data comes as Sting still? Just in case we may find useful use getRegisteredRegionServers method elsewhere, then it would already be returning a list of ServerName.
There was a problem hiding this comment.
Thank @wchevreuil for reviwing, your opinion is indeed better, I have revised it.
|
🎊 +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. |
…s to return ServerName instead of String
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
reviewed
Feb 7, 2021
Comment on lines
+196
to
+197
| return result == null ? null : | ||
| result.stream().map(ServerName::parseServerName).collect(Collectors.toList()); |
There was a problem hiding this comment.
Let's take this opportunity to never return null here. If result is null, let's return emptyList()
There was a problem hiding this comment.
Thank @virajjasani for reviwing.
For method refreshOtherRegionServersList that calls getRegisteredRegionServers, it's different that getting null and empty list. It returns true or false depending on whether the result is null. Getting null means that an exception has occurred or the node does not exist in ZK.
So it is not very recommended to make this change, at least in this issue.
There was a problem hiding this comment.
Ah, I wish we could fix that boolean condition too, but I agree that it doesn't need to be part of this PR. Maybe later sometime.
Thanks
ddupg
added a commit
that referenced
this pull request
Feb 7, 2021
…s to return ServerName instead of String (#2928) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
ddupg
added a commit
that referenced
this pull request
Feb 7, 2021
…s to return ServerName instead of String (#2928) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
ddupg
added a commit
that referenced
this pull request
Feb 7, 2021
…s to return ServerName instead of String (#2928) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
ddupg
added a commit
that referenced
this pull request
Feb 7, 2021
…s to return ServerName instead of String (#2928) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
ddupg
added a commit
that referenced
this pull request
Feb 8, 2021
…s to return ServerName instead of String (#2928) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Feb 24, 2021
…s to return ServerName instead of String (apache#2928) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…s to return ServerName instead of String (apache#2928) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit d81ae67) Change-Id: I9b713ba64dd369fb1aa82800b9d9e5f389d5fd45
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…onServers to return ServerName instead of String (apache#2928)" This reverts commit d81ae67 (cherry picked from commit c4b194a) Change-Id: Ib79a8529a91b02359d58bee344abc987d8ea5603
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…s to return ServerName instead of String (apache#2928) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit b169808) Change-Id: Id290ecf8cdfb8dba03867780c54e2094766806d7
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.
…s to return ServerName instead of String