zpinto commented on code in PR #2772:
URL: https://github.com/apache/helix/pull/2772#discussion_r1546830360
##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -777,6 +850,38 @@ public boolean
validateTopologySettingInInstanceConfig(ClusterConfig clusterConf
return true;
}
+ /**
+ * Overwrite the InstanceConfigProperties from the given InstanceConfig to
this InstanceConfig.
+ * The merge is done by overwriting the properties in this InstanceConfig
with the properties
+ * from the given InstanceConfig. {@link #NON_OVERWRITABLE_PROPERTIES} will
not be overridden.
+ *
+ * @param overwritingInstanceConfig the InstanceConfig to override into this
InstanceConfig
+ */
+ public void overwriteInstanceConfig(InstanceConfig
overwritingInstanceConfig) {
+ for (InstanceConfigProperty keyEnum : InstanceConfigProperty.values()) {
+ if (NON_OVERWRITABLE_PROPERTIES.contains(keyEnum)) {
+ continue;
+ }
+
+ String key = keyEnum.toString();
+ // Add key value pair if it exists in overwritingInstanceConfig and
remove it if it doesn't
+ // exist in overwritingInstanceConfig but does exist in this
InstanceConfig.
+ if
(overwritingInstanceConfig.getRecord().getSimpleFields().containsKey(key)) {
+ _record.setSimpleField(key,
overwritingInstanceConfig.getRecord().getSimpleField(key));
+ } else if (_record.getSimpleFields().containsKey(key)) {
+ _record.getSimpleFields().remove(key);
+ } else if
(overwritingInstanceConfig.getRecord().getListFields().containsKey(key)) {
+ _record.setListField(key,
overwritingInstanceConfig.getRecord().getListField(key));
+ } else if (_record.getListFields().containsKey(key)) {
+ _record.getListFields().remove(key);
+ } else if
(overwritingInstanceConfig.getRecord().getMapFields().containsKey(key)) {
+ _record.setMapField(key,
overwritingInstanceConfig.getRecord().getMapField(key));
+ } else if (_record.getMapFields().containsKey(key)) {
+ _record.getMapFields().remove(key);
+ }
Review Comment:
The reason for this is that if the field has a default when it is not
present in SimpleField, ListField, or MapField map.
Ex.
1. Instance A will overwrite Instance B
2. Instance B has InstanceOperation set to SWAP_IN
3. Instance A has InstanceOperation unset (which means ENABLE)
4. if we addAll on defined keys then Instance B will still have
InstanceOperation set to SWAP_IN which is not right (same issue for other
fields that have defaults if the key doesn't exist)
I have refactored method to be less complex.
It will remove all overwritable fields from this and then set them if the
are in the overwriting InstanceConfig.
--
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: reviews-unsubscr...@helix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
