[GitHub] [helix] desaikomal commented on a diff in pull request #2506: ZkClient add recursive persist listener implementation

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

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


##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -332,15 +333,13 @@ public boolean subscribeStateChanges(ConnectStateChangeListener listener) {
     return true;
   }
 
-  // TODO: add impl and remove UnimplementedException
   @Override
   public boolean subscribeChildChanges(String key, ChildChangeListener listener, boolean skipWatchingNonExistNode) {
-    try {
-      _zkClient.subscribePersistRecursiveWatcher(key, new ChildListenerAdapter(listener));
-    } catch (KeeperException.UnimplementedException e) {
-      LOG.error(e.getLocalizedMessage());
+    if (skipWatchingNonExistNode && exists(key) == null) {
+      return false;
     }
-    return false;
+    _zkClient.subscribePersistRecursiveListener(key, new ChildListenerAdapter(listener));

Review Comment:
   Is there any exception which subscribePersistRecursiveListener can throw?  just wondering 



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -343,17 +343,54 @@ public void subscribeDataChanges(String path, IZkDataListener listener) {
    * Subscribe RecursivePersistListener for a particular path. User can only subscribe when
    * `_usePersistWatcher` is set to true and there is no pre-existing watcher on the path.
    */
-  // TODO: Add impl and remove exception
-  public boolean subscribePersistRecursiveWatcher(String path,
-      RecursivePersistListener recursivePersistListener)
-      throws KeeperException.UnimplementedException {
-    throw new KeeperException.UnimplementedException();
+  public void subscribePersistRecursiveListener(String path,
+      RecursivePersistListener recursivePersistListener) {
+    if (!_usePersistWatcher) {

Review Comment:
   this does throw exception, in that case, should the higher level call return 'false' or we need exception to pass through?



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -343,17 +343,54 @@ public void subscribeDataChanges(String path, IZkDataListener listener) {
    * Subscribe RecursivePersistListener for a particular path. User can only subscribe when
    * `_usePersistWatcher` is set to true and there is no pre-existing watcher on the path.
    */
-  // TODO: Add impl and remove exception
-  public boolean subscribePersistRecursiveWatcher(String path,
-      RecursivePersistListener recursivePersistListener)
-      throws KeeperException.UnimplementedException {
-    throw new KeeperException.UnimplementedException();
+  public void subscribePersistRecursiveListener(String path,
+      RecursivePersistListener recursivePersistListener) {
+    if (!_usePersistWatcher) {
+      throw new UnsupportedOperationException(
+          "Can not subscribe PersistRecursiveWatcher. Persist listener is not enabled.");
+    }
+
+    ManipulateListener addListener = () -> {
+      if (hasChildOrDataListeners(path)) {
+        throw new UnsupportedOperationException(
+            "Can not subscribe PersistRecursiveWatcher. There is an existing listener on " + path);
+      }
+      // subscribe a PERSISTENT_RECURSIVE listener on path. It throws exception if not successful
+      retryUntilConnected(() -> {
+        getConnection().addWatch(path, ZkClient.this, AddWatchMode.PERSISTENT_RECURSIVE);
+        return null;

Review Comment:
   nit: this is for my understanding, why are you returning "null" here?



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1807,6 +1838,23 @@ private void processDataOrChildChange(WatchedEvent event, long notificationTime)
         fireDataChangedEvents(event.getPath(), listeners, OptionalLong.of(notificationTime),
             pathExists, event.getType());
       }
+
+      // fire change event for persist recursive listener
+      if (_usePersistWatcher) {
+        Set<RecursivePersistListener> recListeners =
+            _zkPathRecursiveWatcherTrie.getAllRecursiveListeners(path);
+        if (recListeners != null && !recListeners.isEmpty()) {

Review Comment:
   just my 2cents: not sure if defensive coding is good. it adds to CPU processing something which will never be null.



##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/util/ZkPathRecursiveWatcherTrie.java:
##########
@@ -224,6 +223,28 @@ public void removeRecursiveListener(final String path, RecursivePersistListener
     }
   }
 
+  /**
+   * Return if there is listener on a particular path
+   * @param path
+   * @return
+   */
+  public boolean hasListenerOnPath(String path) {
+    Objects.requireNonNull(path, "Path cannot be null");
+
+    final List<String> pathComponents = split(path);
+    TrieNode cur;
+    synchronized (this) {
+      cur = _rootNode;
+      for (final String element : pathComponents) {
+        cur = cur.getChild(element);
+        if (cur == null) {
+          break;
+        }
+      }
+    }
+    return cur!=null && !cur.getRecursiveListeners().isEmpty();

Review Comment:
   nit: cur != null (space before and after !=)
   



##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/zkclient/TestZkClientPersistWatcher.java:
##########
@@ -116,38 +116,59 @@ 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);
+        .setUsePersistWatcher(true);
     org.apache.helix.zookeeper.impl.client.ZkClient zkClient = builder.build();
     zkClient.setZkSerializer(new BasicZkSerializer(new SerializableSerializer()));
     int count = 100;
     final AtomicInteger[] event_count = {new AtomicInteger(0)};
     final AtomicInteger[] event_count2 = {new AtomicInteger(0)};
-    CountDownLatch countDownLatch1 = new CountDownLatch(count);
-    CountDownLatch countDownLatch2 = new CountDownLatch(count/2);
-    String path = "/base/testZkClientChildChange";
+    CountDownLatch countDownLatch1 = new CountDownLatch(count*4);

Review Comment:
   can you add the comments, the magic *4 is for what reason?



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