[GitHub] [helix] jiajunwang commented on a change in pull request #1019: ConnectionTimeout change to ensure HelixManager initial connection

[GitBox <gi...@apache.org>]

jiajunwang commented on a change in pull request #1019:
URL: https://github.com/apache/helix/pull/1019#discussion_r427763659



##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/ZkClient.java
##########
@@ -62,7 +62,10 @@
   private static Logger LOG = LoggerFactory.getLogger(ZkClient.class);
 
   public static final int DEFAULT_OPERATION_TIMEOUT = Integer.MAX_VALUE;
-  public static final int DEFAULT_CONNECTION_TIMEOUT = 60 * 1000;
+  // Four times of DEFAULT_SESSION_TIMEOUT would give less 1/125 chance of initial connection
+  // to Zookeeper failure assuming one observer in the 5 observer Zookeeper Quorum is down

Review comment:
       1. Please provide the formula instead of one set of input and result in the comment. Not all Helix deployment connects to a ZK which has 5 zk servers.
   2. If we need 4 times of the session timeout, why hardcode here?
   3. Since this configuration depends on the zk server count, I would prefer to specify this in the ZK_CONNECTION_TIMEOUT system property instead of modifying the default connection values. Note that this change might impact all the Helix users that do not specify the ZK_CONNECTION_TIMEOUT, and too long timeout may also cause problems.




----------------------------------------------------------------
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