jiajunwang commented on a change in pull request #1526:
URL: https://github.com/apache/helix/pull/1526#discussion_r544752927
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkConnection.java
##########
@@ -192,4 +234,60 @@ 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;
+ }
+
+ doLookUpGetChildrenMethod();
+
+ LOG.info("Pagination config {}={}, method to be invoked: {}",
+ ZkSystemPropertyKeys.ZK_GETCHILDREN_PAGINATION_DISABLED,
GETCHILDREN_PAGINATION_DISABLED,
+ _getChildrenMethod.getName());
+ }
+
+ private void doLookUpGetChildrenMethod() {
Review comment:
Can we just move it to the lookupGetChildrenMethod() method? I don't see
any other callers of it.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkConnection.java
##########
@@ -192,4 +234,60 @@ public String getServers() {
public void addAuthInfo(String scheme, byte[] auth) {
_zk.addAuthInfo(scheme, auth);
}
+
+ private void lookupGetChildrenMethod() {
Review comment:
One suggestion here, returning the method here so the _getChildrenMethod
is only referred in this method.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkConnection.java
##########
@@ -102,6 +112,7 @@ protected void reconnect(Watcher watcher) throws
InterruptedException {
LOG.debug("Creating new ZookKeeper instance to reconnect to " +
_servers + ".");
_zk = new ZooKeeper(_servers, _sessionTimeOut, watcher);
prevZk.close();
+ _getChildrenMethod = null;
Review comment:
Not sure if this will cause some race condition issues?
For example, the reconnect() cleans up the method here. And in the other
application thread, the operation is about to use this object (with the same
reference), then it will be an NPE.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/constant/ZkSystemPropertyKeys.java
##########
@@ -63,4 +63,18 @@
/** System property key for jute.maxbuffer */
public static final String JUTE_MAXBUFFER = "jute.maxbuffer";
+
+ /**
+ * Setting this property to {@code true} in system properties will force
Helix ZkClient to use
+ * the <b>non-paginated</b> {@code getChildren} API, no matter if zookeeper
supports pagination
+ * or not.
+ * <p>
+ * Given both the zookeeper client and server support <b>paginated</b>
{@code getChildren} API as
+ * a prerequisite, if set to {@code false}, it will enable Helix ZkClient's
{@code getChildren}
+ * API to call zookeeper's <b>paginated</b> {@code getChildren} API.
+ * <p>
+ * The default value is {@code false}.
+ */
+ public static final String ZK_GETCHILDREN_PAGINATION_DISABLED =
Review comment:
On my understanding, this is more for our internal debugging purposes.
If the feature is rock solid then we don't need it.
So the question here is how confident are we about the pagination feature?
And I think the worst case is that we allow this one to temporarily exist.
We need a TODO here to remove it once the feature is stable.
----------------------------------------------------------------
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]
