dasahcc commented on a change in pull request #1129:
URL: https://github.com/apache/helix/pull/1129#discussion_r467272861
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PerInstanceAccessor.java
##########
@@ -308,20 +309,34 @@ public Response
updateInstanceConfig(@PathParam("clusterId") String clusterId,
}
InstanceConfig instanceConfig = new InstanceConfig(record);
ConfigAccessor configAccessor = getConfigAccessor();
+
try {
+ /*
+ * Even if the instance is disabled, non-valid instance topology config
will cause rebalance
+ * failure. We are doing the check whenever user updates InstanceConfig.
+ */
+ if (command == Command.delete || command == Command.update) {
+ validateDeltaTopologySettingInInstanceConfig(clusterId, instanceName,
configAccessor,
Review comment:
Why we dont call this in the switch case? It will make the code more
clear. By doing that, you can let this
validateDeltaTopologySettingInInstanceConfig, return a Config if it is valid.
Otherwise, return null.
So the following code will based on the return result to do the write to ZK.
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -187,26 +187,28 @@ private void joinCluster() {
LOG.info("auto registration is false for cluster" + _clusterName);
}
+ InstanceConfig instanceConfig;
if (!ZKUtil.isInstanceSetup(_zkclient, _clusterName, _instanceName,
_instanceType)) {
if (!autoJoin) {
throw new HelixException("Initial cluster structure is not set up for
instance: "
+ _instanceName + ", instanceType: " + _instanceType);
+ }
+ if (!autoRegistration) {
+ LOG.info(_instanceName + " is auto-joining cluster: " + _clusterName);
+ instanceConfig = HelixUtil.composeInstanceConfig(_instanceName);
} else {
- if (!autoRegistration) {
- LOG.info(_instanceName + " is auto-joining cluster: " +
_clusterName);
- _helixAdmin.addInstance(_clusterName,
HelixUtil.composeInstanceConfig(_instanceName));
- } else {
- LOG.info(_instanceName + " is auto-registering cluster: " +
_clusterName);
- CloudInstanceInformation cloudInstanceInformation =
getCloudInstanceInformation();
- String domain = cloudInstanceInformation
-
.get(CloudInstanceInformation.CloudInstanceField.FAULT_DOMAIN.name()) +
_instanceName;
-
- InstanceConfig instanceConfig =
HelixUtil.composeInstanceConfig(_instanceName);
- instanceConfig.setDomain(domain);
- _helixAdmin.addInstance(_clusterName, instanceConfig);
- }
+ LOG.info(_instanceName + " is auto-registering cluster: " +
_clusterName);
+ CloudInstanceInformation cloudInstanceInformation =
getCloudInstanceInformation();
+ String domain = cloudInstanceInformation
+
.get(CloudInstanceInformation.CloudInstanceField.FAULT_DOMAIN.name()) +
_instanceName;
+ instanceConfig = HelixUtil.composeInstanceConfig(_instanceName);
+ instanceConfig.setDomain(domain);
}
+ _helixAdmin.addInstance(_clusterName, instanceConfig);
}
+ _configAccessor.getInstanceConfig(_clusterName, _instanceName)
+
.validateTopologySettingInInstanceConfig(_configAccessor.getClusterConfig(_clusterName),
Review comment:
Why we dont validate it before we write to ZK?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
