junkaixue commented on code in PR #2609: URL: https://github.com/apache/helix/pull/2609#discussion_r1309463509
########## helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java: ########## @@ -55,12 +58,13 @@ public HelixManagerProperty(Properties helixManagerProperties, CloudConfig cloud } private HelixManagerProperty(String version, long healthReportLatency, - HelixCloudProperty helixCloudProperty, + HelixCloudProperty helixCloudProperty, InstanceConfig defaultInstanceConfig, Review Comment: I prefer not use InstanceConfig. Because InstanceConfig requires you to have an ID for this InstanceConfig object. But when we merge, it is hard for us to determine whether the ID will be overridden or not as the ID is mandatory field. And this field does matter for ZNode creation. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
