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



##########
File path: 
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##########
@@ -69,12 +69,49 @@
   int DEFAULT_SESSION_TIMEOUT = 30 * 1000;
 
   // listener subscription
+  /**
+   * Subscribe the path and the listener will handle child events of the path.
+   * Add exists watch to path if the path does not exist in ZooKeeper server.
+   * WARNING: if the path is created after deletion, users need to 
re-subscribe the path
+   * @param path The zookeeper path
+   * @param listener Instance of {@link IZkDataListener}
+   * The method return null if the path does not exists. Otherwise, return a 
list of children
+   * under the path. The list can be empty if there is no children.
+   */
   List<String> subscribeChildChanges(String path, IZkChildListener listener);
 
+  /**
+   * Subscribe the path and the listener will handle child events of the path
+   * WARNING: if the path is created after deletion, users need to 
re-subscribe the path
+   * @param path The zookeeper path
+   * @param listener Instance of {@link IZkDataListener}
+   * @param skipWatchingNodeNotExist True means not installing any watch if 
path does not exist.

Review comment:
       @kaisun2000 If you agree on the name changing, please change all the 
other parameter names of this option. They are not consistent now.

##########
File path: 
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -748,6 +766,12 @@ public void handleChildChange(String parentPath, 
List<String> currentChilds) {
           // removeListener will call handler.reset(), which in turn call 
invoke() on FINALIZE type
           _manager.removeListener(_propertyKey, _listener);
         } else {
+          if (!isReady()) {
+            // avoid leaking CallbackHandler
+            logger.info("Callbackhandler {} with path {} end of life as not 
ready to avoid leak",

Review comment:
       nit, could you please rephrase this? I think we need to mention the 
re-subscribe is skipped because of the CH end of life.




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

Reply via email to