pkuwm commented on a change in pull request #1129:
URL: https://github.com/apache/helix/pull/1129#discussion_r460490364
##########
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,
+ instanceConfig, command == Command.delete);
+ }
switch (command) {
- case update:
- configAccessor.updateInstanceConfig(clusterId, instanceName,
instanceConfig);
- break;
- case delete:
- HelixConfigScope instanceScope =
- new
HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.PARTICIPANT)
- .forCluster(clusterId).forParticipant(instanceName).build();
- configAccessor.remove(instanceScope, record);
- break;
- default:
- return badRequest(String.format("Unsupported command: %s", command));
+ case update:
+ // The new instanceConfig will be merged with existing one
+ configAccessor.updateInstanceConfig(clusterId, instanceName,
instanceConfig);
+ break;
+ case delete:
+ HelixConfigScope instanceScope =
+ new
HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.PARTICIPANT)
+ .forCluster(clusterId).forParticipant(instanceName).build();
+ configAccessor.remove(instanceScope, record);
+ break;
+ default:
+ return badRequest(String.format("Unsupported command: %s", command));
}
+ } catch (IllegalArgumentException ex) {
+ LOG.error(String.format("Invalid topology setting for Instance : %s.
Fail the config update",
Review comment:
Could make advantage of parameterized logging :)
`LOG.error("Invalid topology setting for Instance : {}", instanceName, ex);`
##########
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:
I suggest changing the method signature a bit: `boolean isDelete` ->
`Command command`. You could put if..else check in this private method.
An nice article to help with clean such boolean parameter :
https://medium.com/@amlcurran/clean-code-the-curse-of-a-boolean-parameter-c237a830b7a3
----------------------------------------------------------------
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]
