[GitHub] [hbase] ddupg commented on a change in pull request #2928: HBASE-25553 It is better for ReplicationTracker.getListOfRegionServer…

2021-02-06 Thread GitBox 


ddupg commented on a change in pull request #2928:
URL: https://github.com/apache/hbase/pull/2928#discussion_r571569523



##
File path: 
hbase-replication/src/main/java/org/apache/hadoop/hbase/replication/ReplicationTrackerZKImpl.java
##
@@ -190,6 +193,7 @@ private boolean refreshOtherRegionServersList(boolean 
watch) {
 } catch (KeeperException e) {
   this.abortable.abort("Get list of registered region servers", e);
 }
-return result;
+return result == null ? null :
+  
result.stream().map(ServerName::parseServerName).collect(Collectors.toList());

Review 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.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] ddupg commented on a change in pull request #2928: HBASE-25553 It is better for ReplicationTracker.getListOfRegionServer…

2021-02-05 Thread GitBox 


ddupg commented on a change in pull request #2928:
URL: https://github.com/apache/hbase/pull/2928#discussion_r570883700



##
File path: 
hbase-replication/src/main/java/org/apache/hadoop/hbase/replication/ReplicationTrackerZKImpl.java
##
@@ -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);

Review comment:
   Thank @wchevreuil for reviwing, your opinion is indeed better, I have 
revised it.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org