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

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

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



##########
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:
       I think we should not even add the method to the HelixManager interface. 1. it is ZK config, conceptually, HelixManager does not have to be implemented by a ZkHelixManager. 2. if we expose more internal implementation details from the public interface, we may be stuck with this design and cannot change anything in the future. This is not a good OO design.
   
   Please see my proposal that I put here earlier.




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