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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
