junkaixue commented on code in PR #2609:
URL: https://github.com/apache/helix/pull/2609#discussion_r1313708994
##########
helix-core/src/main/java/org/apache/helix/util/HelixUtil.java:
##########
@@ -533,21 +533,38 @@ public static long getSystemPropertyAsLong(String
propertyKey, long propertyDefa
/**
* Compose the config for an instance
- * @param instanceName
+ * @param instanceName the unique name of the instance
* @return InstanceConfig
*/
+ @Deprecated
public static InstanceConfig composeInstanceConfig(String instanceName) {
- InstanceConfig instanceConfig = new InstanceConfig(instanceName);
- String hostName = instanceName;
- String port = "";
+ return composeInstanceConfig(instanceName, new InstanceConfig.Builder());
+ }
+
+ /**
+ * Compose the config for an instance with defaults if provided.
+ * @param instanceName the unique name of the instance
+ * @param defaultInstanceConfigBuilder instance config builder filled with
defaults
+ * @return InstanceConfig
+ */
+ public static InstanceConfig composeInstanceConfig(String instanceName,
Review Comment:
If we already have Builder, would assume the parsing logic is already done
and it is overlap with build fields... Let's not have them together.
So I think we can keep the composeInstanceConfig for previous one. And the
build itself is already the logic to generate the instance config. Not need to
have facility function for it.
##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -704,7 +706,7 @@ public static InstanceConfig toInstanceConfig(String
instanceId) {
}
- config.setInstanceEnabled(true);
+ config.setInstanceEnabled(HELIX_ENABLED_DEFAULT_VALUE);
Review Comment:
Let's not add this. Helix basic principal is minimized the writes to
ZNRecord. If it does not show in the ZNRecord, usually we use default value in
get method but not explicit add it to record.
##########
helix-core/src/main/java/org/apache/helix/util/HelixUtil.java:
##########
@@ -533,21 +533,38 @@ public static long getSystemPropertyAsLong(String
propertyKey, long propertyDefa
/**
* Compose the config for an instance
- * @param instanceName
+ * @param instanceName the unique name of the instance
* @return InstanceConfig
*/
+ @Deprecated
public static InstanceConfig composeInstanceConfig(String instanceName) {
Review Comment:
We can keep this as it use just one string to construct very simple instance
config.
Not every company requires complicated instance config.
--
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]
