[GitHub] [helix] kaisun2000 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation

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

kaisun2000 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572499359



##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
     if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {

Review comment:
       This probably should be pointed out in https://github.com/apache/helix/pull/1627. 
   
   if zkAddress is null, we use federatedZkClient, which is not exactly the same as dedicated ZkClient.
   
   My question is more like this: 
   Why do we think that MULTI_ZK_ENABLED and zkAddress == null belongs tot the same handling logic? 
   
   Or put it another way, this condition, "if zkAddress is null for the HelixManager passed to TaskStateModelFactory and multizk is not enabled"? what should be the expected behavior? 
   
   




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