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

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

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



##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -362,6 +363,13 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
    */
   String getMetadataStoreConnectionString();
 
+  /**
+   * @return the RealmAwareZkConnectionConfig usd to create a realm aware zkClient
+   */
+  default RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {

Review comment:
       A high-level comment:
   
   - The overarching principle here is that the underlying ZkClients in the given HelixManager and the ZkClient we are using in TaskStateModelFactory have to have the same connection config.
   - Instead of passing ZkConnectionConfig that returns a null as the default implementation, can we just have them share the underlying ZkClient?
   - This way, we can guarantee that whatever HelixManager sees is exactly the same as what we see in TaskStateModelFactory, and we do not have to worry about managing the lifecycles of two separate ZkClients.




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