This is an automated email from the ASF dual-hosted git repository.
xyuanlu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git
The following commit(s) were added to refs/heads/master by this push:
new e7ada0907 Add cluster config update validation (#3026)
e7ada0907 is described below
commit e7ada0907d7dca67851fbfd60e46340087d8d53d
Author: Xiaxuan Gao <[email protected]>
AuthorDate: Sun May 4 23:01:33 2025 -0700
Add cluster config update validation (#3026)
Add cluster config update validation
---
.../server/resources/helix/ClusterAccessor.java | 58 +++++++++++++++++++
.../helix/rest/server/TestClusterAccessor.java | 66 ++++++++++++++++++++++
2 files changed, 124 insertions(+)
diff --git
a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
index cb56d009c..1e752f3fe 100644
---
a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
+++
b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
@@ -60,6 +60,7 @@ import org.apache.helix.model.ClusterConfig;
import org.apache.helix.model.ControllerHistory;
import org.apache.helix.model.CustomizedStateConfig;
import org.apache.helix.model.HelixConfigScope;
+import org.apache.helix.model.InstanceConfig;
import org.apache.helix.model.LiveInstance;
import org.apache.helix.model.MaintenanceSignal;
import org.apache.helix.model.Message;
@@ -708,9 +709,11 @@ public class ClusterAccessor extends AbstractHelixResource
{
try {
switch (command) {
case update:
+ validateClusterConfigChange(clusterId, configAccessor, config,
command);
configAccessor.updateClusterConfig(clusterId, config);
break;
case delete: {
+ validateClusterConfigChange(clusterId, configAccessor, config,
command);
HelixConfigScope clusterScope =
new
HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.CLUSTER)
.forCluster(clusterId).build();
@@ -1248,4 +1251,59 @@ public class ClusterAccessor extends
AbstractHelixResource {
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());
+ }
+
+ // Only validate the topology related settings if the topology aware is
enabled.
+ if (updatedConfig.isTopologyAwareEnabled()) {
+ if (updatedConfig.getTopology() == null ||
updatedConfig.getFaultZoneType() == null) {
+ throw new IllegalArgumentException(
+ "Topology and fault zone type must be set when topology aware is
enabled.");
+ }
+
+ boolean isTopologyAwareChanged =
+ !oldConfig.isTopologyAwareEnabled() &&
updatedConfig.isTopologyAwareEnabled();
+ boolean isTopologyPathChanged =
+ oldConfig.getTopology() == null || (oldConfig.getTopology() != null
+ && !oldConfig.getTopology().equals(updatedConfig.getTopology()));
+ boolean isFaultZoneTypeChanged =
+ oldConfig.getFaultZoneType() == null ||
(oldConfig.getFaultZoneType() != null
+ &&
!oldConfig.getFaultZoneType().equals(updatedConfig.getFaultZoneType()));
+
+ if (isTopologyAwareChanged || isTopologyPathChanged ||
isFaultZoneTypeChanged) {
+ HelixDataAccessor dataAccessor = getDataAccssor(clusterName);
+ PropertyKey.Builder keyBuilder = dataAccessor.keyBuilder();
+ List<InstanceConfig> instanceConfigs =
dataAccessor.getChildValues(keyBuilder.instanceConfigs(), true);
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ instanceConfig.validateTopologySettingInInstanceConfig(updatedConfig,
+ instanceConfig.getInstanceName());
+ }
+ }
+ }
+ }
}
diff --git
a/helix-rest/src/test/java/org/apache/helix/rest/server/TestClusterAccessor.java
b/helix-rest/src/test/java/org/apache/helix/rest/server/TestClusterAccessor.java
index 4ae7b3dfe..619a78127 100644
---
a/helix-rest/src/test/java/org/apache/helix/rest/server/TestClusterAccessor.java
+++
b/helix-rest/src/test/java/org/apache/helix/rest/server/TestClusterAccessor.java
@@ -90,6 +90,72 @@ public class TestClusterAccessor extends AbstractTestClass {
}
@Test
+ public void testValidateClusterConfigChange() throws IOException {
+ System.out.println("Start test :" + TestHelper.getTestMethodName());
+ String cluster = "TestCluster_1";
+ ClusterConfig config = getClusterConfigFromRest(cluster);
+
+ // Enable the topology aware setting while the instance config does not
have the DOMAIN info
+ {
+ ClusterConfig newConfig = new ClusterConfig(config.getRecord());
+ newConfig.setTopologyAwareEnabled(true);
+ _auditLogger.clearupLogs();
+ Entity entity =
Entity.entity(OBJECT_MAPPER.writeValueAsString(newConfig.getRecord()),
+ MediaType.APPLICATION_JSON_TYPE);
+ post("clusters/" + cluster + "/configs", ImmutableMap.of("command",
Command.update.name()),
+ entity, Response.Status.INTERNAL_SERVER_ERROR.getStatusCode());
+
+ validateAuditLogSize(1);
+ AuditLog auditLog = _auditLogger.getAuditLogs().get(0);
+ Assert.assertEquals(auditLog.getHttpMethod(), HTTPMethods.POST.name());
+ Assert.assertEquals(auditLog.getRequestPath(), "clusters/" + cluster +
"/configs");
+ Assert.assertEquals(auditLog.getExceptions().size(), 1);
+ }
+
+ // Since the topology aware is not enabled, changing the cluster config
should not fail even
+ // if the instance config does not have the DOMAIN info
+ {
+ ClusterConfig newConfig = new ClusterConfig(config.getRecord());
+ newConfig.setFaultZoneType("TestZoneId");
+ newConfig.setTopology("/TestZoneId/instance");
+ newConfig.setTopologyAwareEnabled(false);
+ updateClusterConfigFromRest(cluster, newConfig, Command.update);
+ }
+
+ // Update the topology path string to NULL. This request should go through
since the
+ // topology aware is not enabled.
+ {
+ ClusterConfig newConfig = new ClusterConfig(config.getRecord());
+ newConfig.setTopology(null);
+ newConfig.setFaultZoneType(null);
+ newConfig.setTopologyAwareEnabled(false);
+ updateClusterConfigFromRest(cluster, newConfig, Command.update);
+ }
+
+ // Now update the config while keeping the topology path and fault zone
unchanged (it's still NULL)
+ {
+ ClusterConfig newConfig = new ClusterConfig(config.getClusterName());
+ newConfig.setTopologyAwareEnabled(true);
+ _auditLogger.clearupLogs();
+ Entity entity =
Entity.entity(OBJECT_MAPPER.writeValueAsString(newConfig.getRecord()),
+ MediaType.APPLICATION_JSON_TYPE);
+ post("clusters/" + cluster + "/configs", ImmutableMap.of("command",
Command.update.name()),
+ entity, Response.Status.INTERNAL_SERVER_ERROR.getStatusCode());
+
+ validateAuditLogSize(1);
+ AuditLog auditLog = _auditLogger.getAuditLogs().get(0);
+ Assert.assertEquals(auditLog.getHttpMethod(), HTTPMethods.POST.name());
+ Assert.assertEquals(auditLog.getRequestPath(), "clusters/" + cluster +
"/configs");
+ Assert.assertEquals(auditLog.getExceptions().size(), 1);
+ }
+
+ // Restore the cluster config
+ updateClusterConfigFromRest(cluster, config, Command.update);
+
+ System.out.println("End test :" + TestHelper.getTestMethodName());
+ }
+
+ @Test(dependsOnMethods = "testValidateClusterConfigChange")
public void testGetClusters() throws IOException {
System.out.println("Start test :" + TestHelper.getTestMethodName());
