jiajunwang commented on a change in pull request #1526:
URL: https://github.com/apache/helix/pull/1526#discussion_r545323558
##########
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:
The question here is why the users will ever want to disable it? In
theory, we can make all functions diable-able, but the project will be out of
control. When you debug a user cluster, you need to check dozens of true or
false options, then try to combine these variables in your mind, then check the
log...
I think I cannot manage that kind of support.
So if it is not absolutely necessary, I suggest not making these options
permanent.
Obviously, we need it for now. My point is that it should be temporary.
##########
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:
This is more about style. Overall, private methods shall not directly
refer to the private fields unless it is the method to update them.
There are 2 main benefits, from my perspective:
1. The private method is independent of the private field. If one side is
changed, the code will either not build or work fine. In either case, we will
not introduce hidden bugs.
2. In the case that we want to modulize things further, an independent
private method is much easier to maintain. For example, if you want to make it
a uitl method, or if you want to add more concurrent control there.
##########
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:
I don't get it. The comment was about the if the condition itself. I
don't see any relationship between that suggestion and making it a private
method.
----------------------------------------------------------------
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]
