pkuwm commented on a change in pull request #1526:
URL: https://github.com/apache/helix/pull/1526#discussion_r535744166
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkConnection.java
##########
@@ -132,10 +139,41 @@ public boolean exists(String path, boolean watch) throws
KeeperException, Interr
return _zk.exists(path, watch) != null;
}
+ /**
+ * Returns a list of children of the given path.
+ * <p>
+ * If the watch is non-null and the call is successful (no exception is
thrown),
+ * a watch will be left on the node with the given path.
+ * <p>
+ * The implementation uses java reflection to check whether the native zk
supports
+ * paginated getChildren API:
+ * <p>- if yes, and {@link #GETCHILDREN_PAGINATION_DISABLED} is false, call
the paginated API;
+ * <p>- otherwise, fall back to the non-paginated API.
+ *
+ * @param path the path of the node
+ * @param watch a boolean flag to indicate whether the watch should be added
to the node
+ * @return a list of children of the given path
+ * @throws KeeperException if the server signals an error with a non-zero
error code
+ * @throws InterruptedException if the server transaction is interrupted
+ */
@Override
public List<String> getChildren(final String path, final boolean watch)
throws KeeperException, InterruptedException {
- return _zk.getChildren(path, watch);
+ lookupGetChildrenMethod();
Review comment:
1. Because it will change back the method if we just executed a fallback
how is it changed back the method? I mean we have if (method != null) return so
if the method is already cached, we don’t go further to parse the zookeeper
class and change the method cache.
2. The reason we need to reset the cache in reconnect is, when server
upgrades to pagination and session expires, it could have a chance to detect
server pagination is available. This is the benefit. If we don’t want it, reset
in reconnect is unnecessary.
Now, there are multiple places to change the method:
1. reconnect / close, invalidate the cache: method = null
2. in getChildren:
a. if method == null: lookup. else: continue to use the cached method.
b. if method is unimplemented in server, fallback
With this strategy, we only have one place in getChildren to lookup the
paginated method. Only the case is not able to dynamically detected: server
upgrades to pagination from non pagination and there is no session
re-establish. For this, we expect clients to restart to be able to use
pagination.
@jiajunwang I am not sure if I fully get your point and answer it. Let me
know if I don't. Thanks for the point.
----------------------------------------------------------------
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]
