jiajunwang commented on a change in pull request #1526:
URL: https://github.com/apache/helix/pull/1526#discussion_r535620331
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkConnection.java
##########
@@ -192,4 +230,53 @@ public String getServers() {
public void addAuthInfo(String scheme, byte[] auth) {
_zk.addAuthInfo(scheme, auth);
}
+
+ private void lookupGetChildrenMethod() {
+ if (_getChildrenMethod != null) {
+ // Method is already cached.
+ return;
+ }
+ try {
+ if (GETCHILDREN_PAGINATION_DISABLED) {
+ lookupNonPaginatedGetChildren();
+ } else {
+ // Lookup the paginated getChildren API
+ _getChildrenMethod =
+ ZooKeeper.class.getMethod("getAllChildrenPaginated", String.class,
boolean.class);
+ }
+ } catch (NoSuchMethodException e1) {
+ // Pagination API is not supported, fall back to non-paginated API
+ lookupNonPaginatedGetChildren();
+ }
+ LOG.info("Pagination config {}={}, method to be invoked: {}",
+ ZkSystemPropertyKeys.ZK_GETCHILDREN_PAGINATION_DISABLED,
GETCHILDREN_PAGINATION_DISABLED,
+ _getChildrenMethod.getName());
+ }
+
+ private void lookupNonPaginatedGetChildren() {
+ try {
+ _getChildrenMethod = ZooKeeper.class.getMethod("getChildren",
String.class, boolean.class);
+ } catch (NoSuchMethodException e2) {
+ // We should not expect this exception here.
+ LOG.error("getChildren is not supported in this zookeeper version!");
+ }
+ }
+
+ private void handleInvokedMethodException(Throwable cause)
+ throws KeeperException, InterruptedException {
+ if (cause instanceof KeeperException.UnimplementedException) {
+ LOG.warn("Paginated getChildren is unimplemented in ZK server! "
+ + "Falling back to non-paginated getChildren");
+ lookupNonPaginatedGetChildren();
Review comment:
I think the retry should be done here with some fallback logic (which
changes the cached method). Otherwise, the caller's retry will fail too, right?
So why bother?
##########
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:
Please consider this point together with the question that if we want to
have a fallback logic in case the server-side is actually on the older version.
1. if fallback logic is required, then always check here is not a good idea.
Because it will change back the method if we just executed a fallback. Instead,
we need to adjust the cached method in multiple places following the slightly
different logic depends on whether the proposed method is executable or not.
2. if fallback logic is not required, then I don't think we need to reset
the cached method on reconnect. The only possibility is our user uses
reflection to replace the native zookeeper class at runtime, right? If they are
going to that route, then the cached method won't be valid anyway. And we
cannot use cache.
----------------------------------------------------------------
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]
