junkaixue commented on code in PR #2588:
URL: https://github.com/apache/helix/pull/2588#discussion_r1294076992
##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -329,6 +330,17 @@ public long getInstanceEnabledTime() {
return
_record.getLongField(InstanceConfigProperty.HELIX_ENABLED_TIMESTAMP.name(), -1);
}
+ public void setInstanceOperation(InstanceConstants.InstanceOperation
operation) {
+ // TODO: also setInstanceEnabled after sanity check.
+
+
_record.setSimpleField(InstanceConfigProperty.INSTANCE_OPERATION.toString(),
Review Comment:
use InstanceConfigProperty.INSTANCE_OPERATION.name().
Not use toString anymore. It is not consistent with line 341
##########
helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java:
##########
@@ -8,4 +8,12 @@ public enum InstanceDisabledType {
USER_OPERATION,
DEFAULT_INSTANCE_DISABLE_TYPE
}
+
+ public enum InstanceOperation {
+ EVACUATE,
+ SWAP_IN,
+ SWAP_OUT,
+ ENABLE,
+ DISABLE;
Review Comment:
Why we need both enable/disable... Suggest just have disable.
##########
helix-rest/src/test/java/org/apache/helix/rest/server/TestPerInstanceAccessor.java:
##########
@@ -486,6 +486,39 @@ public void updateInstance() throws IOException {
Assert.assertEquals(new HashSet<>(instanceConfig.getDisabledPartitionsMap()
.get(CLUSTER_NAME + dbName.substring(0, dbName.length() - 1))),
new HashSet<>(Arrays.asList(CLUSTER_NAME + dbName + "0", CLUSTER_NAME
+ dbName + "3")));
+
+ // test set instance operation
+ new
JerseyUriRequestBuilder("clusters/{}/instances/{}?command=setInstanceOperation&instanceOperation=EVACUATE")
+ .format(CLUSTER_NAME, INSTANCE_NAME).post(this, entity);
+ instanceConfig = _configAccessor.getInstanceConfig(CLUSTER_NAME,
INSTANCE_NAME);
+ Assert.assertEquals(
+ instanceConfig.getInstanceOperation(),
InstanceConstants.InstanceOperation.EVACUATE.toString());
+ new
JerseyUriRequestBuilder("clusters/{}/instances/{}?command=setInstanceOperation&instanceOperation=INVALIDOP")
+
.expectedReturnStatusCode(Response.Status.NOT_FOUND.getStatusCode()).format(CLUSTER_NAME,
INSTANCE_NAME).post(this, entity);
+ new
JerseyUriRequestBuilder("clusters/{}/instances/{}?command=setInstanceOperation")
+
.expectedReturnStatusCode(Response.Status.BAD_REQUEST.getStatusCode()).format(CLUSTER_NAME,
INSTANCE_NAME).post(this, entity);
+ // TODO: enable the following test when we add sanity check.
+ /*
Review Comment:
Pls remove them if they are not be part of official code.
##########
helix-core/src/test/java/org/apache/helix/mock/MockHelixAdmin.java:
##########
@@ -306,6 +305,32 @@ public void enableInstance(String clusterName,
List<String> instances, boolean e
}
+ @Override
+ public void setInstanceOperation(String clusterName, String instanceName,
+ InstanceConstants.InstanceOperation instanceOperation) {
+
+ String path = PropertyPathBuilder.instanceConfig(clusterName,
instanceName);
+
+ if (!_baseDataAccessor.exists(path, 0)) {
+ throw new HelixException(
+ "Cluster " + clusterName + ", instance: " + instanceName + ",
instance config does not exist");
+ }
+
+ _baseDataAccessor.update(path, new DataUpdater<ZNRecord>() {
+ @Override
+ public ZNRecord update(ZNRecord currentData) {
+ if (currentData == null) {
+ throw new HelixException(
+ "Cluster: " + clusterName + ", instance: " + instanceName + ",
participant config is null");
+ }
+
+ InstanceConfig config = new InstanceConfig(currentData);
+ config.setInstanceOperation(instanceOperation); // we set instance
enabled in instance config
+ return config.getRecord();
Review Comment:
nit: This is not used anywhere. If this function cannot be verified , why
not just print a log instead of having this large code.
--
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]
