[GitHub] [helix] desaikomal commented on a diff in pull request #2465: Add exception for register ZK one time watcher when _usePersistWatcher flag is on

["desaikomal (via GitHub)" <gi...@apache.org>]

desaikomal commented on code in PR #2465:
URL: https://github.com/apache/helix/pull/2465#discussion_r1181662867


##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1360,6 +1360,13 @@ public List<String> getChildren(String path) {
   }
 
   protected List<String> getChildren(final String path, final boolean watch) {
+    // Throws exception when try to subscribe watch when using _usePersistWatcher. When ZkClient
+    // is subscribed as persist watcher, resubscribing the same object as onw time watcher will
+    // over write the persist watcher causing missing following event.
+    if (_usePersistWatcher && watch) {

Review Comment:
   is this change in API contract? If not, we should be good.



##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/TestZooKeeperConnection.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.helix.zookeeper.impl;
+
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.helix.zookeeper.zkclient.IZkConnection;
+import org.apache.helix.zookeeper.zkclient.ZkClient;
+import org.apache.zookeeper.AddWatchMode;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZooDefs;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TestZooKeeperConnection extends ZkTestBase {
+  final int count = 100;
+  final int[] get_count = {0};
+  CountDownLatch countDownLatch = new CountDownLatch(count*2);
+
+
+  /*
+  This function tests persist watchers' behavior in {@link org.apache.helix.zookeeper.zkclient.ZkConnection}
+  1. Register a persist watcher on a path and create 100 children Znode, edit the ZNode for 100 times.
+  Expecting 200 events.
+  2. register a one time listener on the path. Make the same change and count the total number of event.

Review Comment:
   shouldn't the one-time listener throw exception?



##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/TestZooKeeperConnection.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.helix.zookeeper.impl;
+
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.helix.zookeeper.zkclient.IZkConnection;
+import org.apache.helix.zookeeper.zkclient.ZkClient;
+import org.apache.zookeeper.AddWatchMode;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZooDefs;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TestZooKeeperConnection extends ZkTestBase {
+  final int count = 100;
+  final int[] get_count = {0};
+  CountDownLatch countDownLatch = new CountDownLatch(count*2);
+
+
+  /*
+  This function tests persist watchers' behavior in {@link org.apache.helix.zookeeper.zkclient.ZkConnection}
+  1. Register a persist watcher on a path and create 100 children Znode, edit the ZNode for 100 times.
+  Expecting 200 events.
+  2. register a one time listener on the path. Make the same change and count the total number of event.
+  */
+  @Test
+  void testPersistWatcher() throws Exception {
+    Watcher watcher1 = new PersistWatcher();
+    ZkClient zkClient =   new org.apache.helix.zookeeper.impl.client.ZkClient(ZK_ADDR);
+    IZkConnection _zk = zkClient.getConnection();
+    String path="/testPersistWatcher";
+    _zk.create(path, null, ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+    // register a persist listener on a path, change the ZNode 100 times, create 100 child ZNode,
+    // and expecting 200 events
+    _zk.addWatch(path, watcher1, AddWatchMode.PERSISTENT);
+    for (int i=0; i<count; ++i) {

Review Comment:
   nit: just curious - style guide doesn't complain about spacing etc?



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