[GitHub] [helix] junkaixue commented on a diff in pull request #2325: Refactor zookeeper-api module with more flexible watcher and connection setup

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

junkaixue commented on code in PR #2325:
URL: https://github.com/apache/helix/pull/2325#discussion_r1061038410


##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/ZkClient.java:
##########
@@ -85,7 +88,20 @@ public class ZkClient extends org.apache.helix.zookeeper.zkclient.ZkClient imple
    *            The JMX bean name will be: HelixZkClient.monitorType.monitorKey.monitorInstanceName.
    * @param monitorRootPathOnly
    *            Should only stat of access to root path be reported to JMX bean or path-specific stat be reported too.
+   * @param connectOnInit true if connect to ZK during initialization, otherwise user will need to call connect
+   *                      explicitly before talking to ZK.
    */
+  public ZkClient(IZkConnection zkConnection, Watcher watcher, int connectionTimeout, long operationRetryTimeout,
+      PathBasedZkSerializer zkSerializer,
+      String monitorType, String monitorKey, String monitorInstanceName,
+      boolean monitorRootPathOnly, boolean connectOnInit) {
+    super(zkConnection, watcher, operationRetryTimeout, zkSerializer, monitorType, monitorKey, monitorInstanceName,
+        monitorRootPathOnly);
+    if (connectOnInit) {
+      connect(connectionTimeout, watcher);
+    }

Review Comment:
   I would suggest get these logic another different PR. One PR just for pure refactoring not changing logic / adding things.
   
   The other one is for new features. This looks like a new feature instead of refactoring.



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -239,16 +250,8 @@ protected ZkClient(IZkConnection zkConnection, int connectionTimeout, long opera
     } else {
       LOG.info("ZkClient monitor key or type is not provided. Skip monitoring.");
     }
-
-    connect(connectionTimeout, this);
-
-    try {
-      if (_monitor != null) {
-        _monitor.register();
-      }
-    } catch (JMException e){
-      LOG.error("Error in creating ZkClientMonitor", e);
-    }
+    // set the zookeeper watcher for notification, use "this" native zkclient instance by default if not explicitly set
+    _watcher = watcher == null ? this : watcher;

Review Comment:
   This changes the behavior, right?



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1566,7 +1569,7 @@ private void reconnect() {
     getEventLock().lock();
     try {
       ZkConnection connection = ((ZkConnection) getConnection());
-      connection.reconnect(this);
+      connection.reconnect(_watcher);

Review Comment:
   What if the passed in watcher is not null? Then it used the watcher? I am not sure whether this is correct interpretation of watcher. 



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

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org