You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "qqu0127 (via GitHub)" <gi...@apache.org> on 2023/05/22 16:46:34 UTC
[GitHub] [helix] qqu0127 commented on a diff in pull request #2495: Add recursive persist listener API in ZkClient and test native ZK
qqu0127 commented on code in PR #2495:
URL: https://github.com/apache/helix/pull/2495#discussion_r1200756167
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -334,6 +334,11 @@ public boolean subscribeStateChanges(ConnectStateChangeListener listener) {
@Override
public boolean subscribeChildChanges(String key, ChildChangeListener listener, boolean skipWatchingNonExistNode) {
+ try {
+ _zkClient.subscribePersistRecursiveWatcher(key, new ChildListenerAdapter(listener));
+ } catch (KeeperException.UnimplementedException e) {
+ e.printStackTrace();
Review Comment:
Let's use the logger
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -349,7 +354,10 @@ public void unsubscribeDirectChildChange(String key, DirectChildChangeListener l
@Override
public void unsubscribeChildChanges(String key, ChildChangeListener listener) {
-
+ try{
+ _zkClient.unsubscribePersistRecursiveWatcher(key, new ChildListenerAdapter(listener));} catch (KeeperException.UnimplementedException e) {
+ e.printStackTrace();
Review Comment:
same
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -349,7 +354,10 @@ public void unsubscribeDirectChildChange(String key, DirectChildChangeListener l
@Override
public void unsubscribeChildChanges(String key, ChildChangeListener listener) {
-
+ try{
+ _zkClient.unsubscribePersistRecursiveWatcher(key, new ChildListenerAdapter(listener));} catch (KeeperException.UnimplementedException e) {
Review Comment:
style: space after try and new line for catch
##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/TestZooKeeperConnection.java:
##########
@@ -57,6 +59,40 @@ void testPersistWatcher() throws Exception {
zkClient.close();
}
+ @Test (dependsOnMethods = "testPersistWatcher")
+ void testRecursivePersistWatcherWithOneTimeWatcher() throws Exception {
+ // reset counter
+ get_count[0] = 0;
+ Watcher watcher1 = new PersistRecurWatcher();
+ ZkClient zkClient = new org.apache.helix.zookeeper.impl.client.ZkClient(ZK_ADDR);
+ IZkConnection _zk = zkClient.getConnection();
+ String path="/testRecursivePersistWatcher";
+ _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_RECURSIVE);
+ for (int i=0; i<count; ++i) {
+ _zk.writeData(path, "datat".getBytes(), -1);
+ _zk.create(path+"/c1_" +i, null, ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ _zk.create(path+"/c1_" +i + "/c2", null, ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ Assert.assertTrue(countDownLatch2.await(50000, TimeUnit.MILLISECONDS));
+
+ // register a one time listener on the path. Count the total number of event.
+ // ZK will over write the persist watcher and only trigger event once for child and data change.
+ _zk.readData(path, null, true);
+ _zk.getChildren(path, true);
+ for (int i=0; i<200; ++i) {
+ _zk.writeData(path, ("datat"+i).getBytes(), -1);
+ _zk.create(path+"/c2_" +i, null, ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ Assert.assertTrue(TestHelper.verify(() -> {
+ return (get_count[0] == 302);
Review Comment:
for my own understanding, why 302?
##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/TestZooKeeperConnection.java:
##########
@@ -57,6 +59,40 @@ void testPersistWatcher() throws Exception {
zkClient.close();
}
+ @Test (dependsOnMethods = "testPersistWatcher")
+ void testRecursivePersistWatcherWithOneTimeWatcher() throws Exception {
+ // reset counter
+ get_count[0] = 0;
Review Comment:
This is not thread safe. Why not use AtomicInteger?
##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/zkclient/TestZkClientPersistWatcher.java:
##########
@@ -108,6 +110,47 @@ public void handleChildChange(String parentPath, List<String> currentChilds)
zkClient.close();
}
+ @Test(dependsOnMethods = "testZkClientChildChange")
+ void testZkClientPersistRecursiveChange() throws Exception {
+ org.apache.helix.zookeeper.impl.client.ZkClient.Builder builder =
+ new org.apache.helix.zookeeper.impl.client.ZkClient.Builder();
+ builder.setZkServer(ZkTestBase.ZK_ADDR).setMonitorRootPathOnly(false)
+ .setUsePersistWatcher(false);
+ org.apache.helix.zookeeper.impl.client.ZkClient zkClient = builder.build();
+ zkClient.setZkSerializer(new BasicZkSerializer(new SerializableSerializer()));
+ int count = 100;
+ final int[] event_count = {0};
+ final int[] event_count2 = {0};
Review Comment:
same, better to sue atomic integer
##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/zkclient/TestZkClientPersistWatcher.java:
##########
@@ -108,6 +110,47 @@ public void handleChildChange(String parentPath, List<String> currentChilds)
zkClient.close();
}
+ @Test(dependsOnMethods = "testZkClientChildChange")
+ void testZkClientPersistRecursiveChange() throws Exception {
Review Comment:
May I suggest we have test coverage on:
1. persistent watch register and removal
2. Concurrent tests with multiple watchers on same path
--
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