[GitHub] [hbase] wchevreuil commented on a change in pull request #2430: HBASE-25074 Refactor ReplicationSinkManager: reduce code and make it …

[GitBox <gi...@apache.org>]

wchevreuil commented on a change in pull request #2430:
URL: https://github.com/apache/hbase/pull/2430#discussion_r491893347



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/HBaseReplicationEndpoint.java
##########
@@ -63,7 +121,7 @@ protected synchronized void disconnect() {
    * A private method used to re-establish a zookeeper session with a peer cluster.
    * @param ke
    */
-  protected void reconnect(KeeperException ke) {
+  private void reconnect(KeeperException ke) {

Review comment:
       Can we keep all accessors to ZKWatcher `protected` or at least, `package private`? In some cases, custom HBaseReplicationEndpoint implementations may need to access those.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/HBaseReplicationEndpoint.java
##########
@@ -150,13 +202,19 @@ public boolean isAborted() {
 
   /**
    * Get the list of all the region servers from the specified peer
-   * @param zkw zk connection to use
+   *
    * @return list of region server addresses or an empty list if the slave is unavailable
    */
-  protected static List<ServerName> fetchSlavesAddresses(ZKWatcher zkw)
-      throws KeeperException {
-    List<String> children = ZKUtil.listChildrenAndWatchForNewChildren(zkw,
-            zkw.getZNodePaths().rsZNode);
+  private List<ServerName> fetchSlavesAddresses() {

Review comment:
       Same as my previous comment. Would be nice to allow for custom implementations to access this method. 




----------------------------------------------------------------
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:
users@infra.apache.org