xyuanlu commented on code in PR #3026:
URL: https://github.com/apache/helix/pull/3026#discussion_r2067065709


##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java:
##########
@@ -1248,4 +1251,47 @@ private Map<String, Object> getControllerHistory(String 
clusterId,
   private AclRegister getAclRegister() {
     return (AclRegister) 
_application.getProperties().get(ContextPropertyKeys.ACL_REGISTER.name());
   }
+
+  /**
+   * Validates the changes to the cluster configuration.
+   *
+   * Specifically checks changes related to topology settings. If topology 
settings are updated,
+   * ensures all instance configurations align with the new cluster 
configuration.
+   *
+   * @param clusterName Name of the cluster to validate.
+   * @param configAccessor Accessor to retrieve and update cluster 
configurations.
+   * @param newClusterConfig The new cluster configuration to validate against.
+   * @param command Type of command triggering this validation (e.g., update, 
delete).
+   */
+  private void validateClusterConfigChange(String clusterName, ConfigAccessor 
configAccessor,
+      ClusterConfig newClusterConfig, Command command) {
+    ClusterConfig oldConfig = configAccessor.getClusterConfig(clusterName);
+    ClusterConfig updatedConfig = configAccessor.getClusterConfig(clusterName);
+
+    if (command == Command.delete) {
+      // Since the topology related setting is only in the simple field, we 
don't need to validate
+      // other fields.
+      for (Map.Entry<String, String> entry : 
newClusterConfig.getRecord().getSimpleFields()
+          .entrySet()) {
+        updatedConfig.getRecord().getSimpleFields().remove(entry.getKey());
+      }
+    } else {
+      updatedConfig.getRecord().update(newClusterConfig.getRecord());
+    }
+
+    boolean isTopologySettingChanged =
+        (!oldConfig.isTopologyAwareEnabled() && 
updatedConfig.isTopologyAwareEnabled())

Review Comment:
   I think we can not assume the old config is valid here. So if we change from 
oldConfig.getTopology() from null to not null, when `isTopologyAwareEnabled` is 
true, we also need to check. 



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

Reply via email to