You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/11/23 23:44:28 UTC
[GitHub] [helix] jiajunwang commented on a change in pull request #1526: Leverage zk paginated getChildren API for ZkClient to fetch a large number of children
jiajunwang commented on a change in pull request #1526:
URL: https://github.com/apache/helix/pull/1526#discussion_r529064541
##########
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:
Instead of calling this every time when client getChildren, can we just initialize the method when constructing the ZkConnection?
##########
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();
+ }
Review comment:
nit, how about the following logic?
if (! GETCHILDREN_PAGINATION_DISABLED) {
try {
_getChildrenMethod =
ZooKeeper.class.getMethod("getAllChildrenPaginated", String.class, boolean.class);
return;
} catch (...) {
log...
}
}
lookupNonPaginatedGetChildren();
return;
##########
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:
In this case, do you want to retry the operation?
##########
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!");
Review comment:
Just throw an exception. Fail earlier to block the whole logic from starting up. This is much safer than allowing a partially working client.
##########
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)
Review comment:
Just to confirm, as we discussed, if the ZK server is upgraded in between, the client shall be able to leverage the new API immediately, right? In this case, I assume we expect the ZkConnection object to be recreated. And I believe this is the case when we handle new session creation.
But what if the server upgrade is fast and the session is not expired? In this case, since you update the method here (and fallback to the older one), then the client will never use pagination call unless we restart the client.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org