[GitHub] [helix] jiajunwang commented on a change in pull request #1035: [WIP] Fix leaking Zk path watch and Callbackhandler issue

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

jiajunwang commented on a change in pull request #1035:
URL: https://github.com/apache/helix/pull/1035#discussion_r431622902



##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -257,12 +274,22 @@ public void subscribeDataChanges(String path, IZkDataListener listener) {
         }
       }
     }
-    watchForData(path);
+

Review comment:
       I don't think we can switch this order here. Immediately after the watcher is installed, the event might come. If it happens before _dataListener put is done, then we miss that event.

##########
File path: helix-core/src/test/java/org/apache/helix/integration/manager/ClusterSpectatorManager.java
##########
@@ -0,0 +1,82 @@
+package org.apache.helix.integration.manager;
+
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+
+import org.apache.helix.InstanceType;
+import org.apache.helix.manager.zk.CallbackHandler;
+import org.apache.helix.manager.zk.ZKHelixManager;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class ClusterSpectatorManager extends ZKHelixManager implements Runnable, ZkTestManager {

Review comment:
       This is mostly duplicate with ClusterControllerManager. I think we can create a parent class for both classes so as to avoid duplicate code.

##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1074,6 +1101,34 @@ private Stat getStat(final String path, final boolean watch) {
     }
   }
 
+  /*
+   * Install watch if there is such node and return the stat
+   * Don't install watch if there is no such node and return null
+   *
+   * Use ZooKeeper native api getData() as underlying method
+   */
+  private Stat getStat2(final String path) {

Review comment:
       You can avoid this awkwardly named method by modifying the exiting private getStat() method.
   Just add a parameter to indicate if it should do getData() or exists() internally. That will also help to reduce duplicate code.

##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1858,24 +1923,47 @@ public Object call() throws Exception {
     });
   }
 
+  private boolean watchForData(final String path, boolean skipWatchingNodeNotExist) {

Review comment:
       code style-wise, we should implement the original watchForData method using this new one to avoid duplicate logic.




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



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