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
commit 059f1f60cff4531ad8b5e644f75c8942aa0dc154 Author: xyuanlu <[email protected]> AuthorDate: Mon Aug 21 10:03:50 2023 -0700 Instance Evacuation operation sanity check (#2598) Instance Evacuation operation sanity check --- .../org/apache/helix/model/InstanceConfig.java | 31 ++++++++++++++++++---- .../helix/rest/server/TestPerInstanceAccessor.java | 21 +++++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java b/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java index bdd49cafb..fdcb74517 100644 --- a/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java +++ b/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java @@ -264,9 +264,22 @@ public class InstanceConfig extends HelixProperty { * @param enabled true to enable, false to disable */ public void setInstanceEnabled(boolean enabled) { + // set instance operation only when we need to change InstanceEnabled value. + // When enabling an instance where current HELIX_ENABLED is false, we update INSTANCE_OPERATION to 'ENABLE' + // When disabling and instance where current HELIX_ENABLED is false, we overwrite what current operation and + // update INSTANCE_OPERATION to 'DISABLE'. + String instanceOperationKey = InstanceConfigProperty.INSTANCE_OPERATION.toString(); + if (enabled != getInstanceEnabled()) { + _record.setSimpleField(instanceOperationKey, + enabled ? InstanceConstants.InstanceOperation.ENABLE.name() + : InstanceConstants.InstanceOperation.DISABLE.name()); + } + setInstanceEnabledHelper(enabled); + } + + private void setInstanceEnabledHelper(boolean enabled) { _record.setBooleanField(InstanceConfigProperty.HELIX_ENABLED.toString(), enabled); - _record.setLongField(InstanceConfigProperty.HELIX_ENABLED_TIMESTAMP.name(), - System.currentTimeMillis()); + _record.setLongField(InstanceConfigProperty.HELIX_ENABLED_TIMESTAMP.name(), System.currentTimeMillis()); if (enabled) { resetInstanceDisabledTypeAndReason(); } @@ -331,10 +344,17 @@ public class InstanceConfig extends HelixProperty { } public void setInstanceOperation(InstanceConstants.InstanceOperation operation) { - // TODO: also setInstanceEnabled after sanity check. + if (operation != InstanceConstants.InstanceOperation.DISABLE + && operation != InstanceConstants.InstanceOperation.ENABLE ){ + if( !getInstanceEnabled()) { + throw new HelixException( + "setting non enable/disable operation (e.g. evacuate, swap) to helix disabled instance is not allowed"); + } + } else { + setInstanceEnabledHelper(operation == InstanceConstants.InstanceOperation.ENABLE); + } - _record.setSimpleField(InstanceConfigProperty.INSTANCE_OPERATION.name(), - operation.name()); + _record.setSimpleField(InstanceConfigProperty.INSTANCE_OPERATION.toString(), operation.toString()); } public String getInstanceOperation() { @@ -361,6 +381,7 @@ public class InstanceConfig extends HelixProperty { /** * Check if this instance is enabled for a given partition + * * @param partition the partition name to check * @return true if the instance is enabled for the partition, false otherwise */ diff --git a/helix-rest/src/test/java/org/apache/helix/rest/server/TestPerInstanceAccessor.java b/helix-rest/src/test/java/org/apache/helix/rest/server/TestPerInstanceAccessor.java index fe4e19906..d721c372a 100644 --- a/helix-rest/src/test/java/org/apache/helix/rest/server/TestPerInstanceAccessor.java +++ b/helix-rest/src/test/java/org/apache/helix/rest/server/TestPerInstanceAccessor.java @@ -497,6 +497,27 @@ public class TestPerInstanceAccessor extends AbstractTestClass { .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. + // set operation to be DISABLE + new JerseyUriRequestBuilder("clusters/{}/instances/{}?command=setInstanceOperation&instanceOperation=DISABLE") + .format(CLUSTER_NAME, INSTANCE_NAME).post(this, entity); + instanceConfig = _configAccessor.getInstanceConfig(CLUSTER_NAME, INSTANCE_NAME); + Assert.assertEquals( + instanceConfig.getInstanceOperation(), InstanceConstants.InstanceOperation.DISABLE.toString()); + Assert.assertTrue(!instanceConfig.getInstanceEnabled()); + + // set operation to EVACUATE, expect error + new JerseyUriRequestBuilder("clusters/{}/instances/{}?command=setInstanceOperation&instanceOperation=EVACUATE") + .expectedReturnStatusCode(Response.Status.BAD_REQUEST.getStatusCode()) + .format(CLUSTER_NAME, INSTANCE_NAME).post(this, entity); + // set back to enable + new JerseyUriRequestBuilder("clusters/{}/instances/{}?command=setInstanceOperation&instanceOperation=ENABLE") + .format(CLUSTER_NAME, INSTANCE_NAME).post(this, entity); + instanceConfig = _configAccessor.getInstanceConfig(CLUSTER_NAME, INSTANCE_NAME); + Assert.assertEquals( + instanceConfig.getInstanceOperation(), InstanceConstants.InstanceOperation.ENABLE.toString()); + Assert.assertTrue(instanceConfig.getInstanceEnabled()); + System.out.println("End test :" + TestHelper.getTestMethodName()); }
