From 7391e6ebaf49e82977bc1213b1cd5123d79cab61 Mon Sep 17 00:00:00 2001 From: xyuanlu Date: Tue, 26 Nov 2024 15:15:40 -0800 Subject: [PATCH 1/4] fix enabled --- .../org/apache/helix/model/InstanceConfig.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 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 1b3acd68d6..c90efb6a38 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 @@ -642,8 +642,8 @@ public InstanceOperation getInstanceOperation() { .build(); } - // Always respect the HELIX_ENABLED being set to false when instance operation is unset - // for backwards compatibility. + // if instance operation is not DISABLED, we always respect the HELIX_ENABLED being set to false + // when instance operation is unset for backwards compatibility. if (!_record.getBooleanField(InstanceConfigProperty.HELIX_ENABLED.name(), HELIX_ENABLED_DEFAULT_VALUE) && (InstanceConstants.INSTANCE_DISABLED_OVERRIDABLE_OPERATIONS.contains( @@ -656,6 +656,17 @@ public InstanceOperation getInstanceOperation() { .build(); } + // if instance operation id DISABLE, we override it to ENABLE if instance is enabled + if (activeInstanceOperation.getOperation().equals(InstanceConstants.InstanceOperation.DISABLE)) { + // it is not likely that instanceEnabled is unset, because when we set operation to disable, + // we always set instanceEnabled to false + // If instance is enabled by old version helix (not having instance operation), the instance config + // will have HELIX_ENABLED set to true. In this case, we should override the instance operation to ENABLE + if ("true".equals(_record.getSimpleField(InstanceConfigProperty.HELIX_ENABLED.name()))) { + return new InstanceOperation.Builder().setOperation(InstanceConstants.InstanceOperation.ENABLE).build(); + } + } + return activeInstanceOperation; } From 4ba107792ee88176a4277924834b11f80e23f3c6 Mon Sep 17 00:00:00 2001 From: xyuanlu Date: Mon, 2 Dec 2024 12:24:28 -0800 Subject: [PATCH 2/4] add test --- .../org/apache/helix/model/InstanceConfig.java | 4 ++-- .../apache/helix/model/TestInstanceConfig.java | 17 ++++++++++++++++- 2 files changed, 18 insertions(+), 3 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 c90efb6a38..508e3eeff2 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 @@ -658,8 +658,8 @@ public InstanceOperation getInstanceOperation() { // if instance operation id DISABLE, we override it to ENABLE if instance is enabled if (activeInstanceOperation.getOperation().equals(InstanceConstants.InstanceOperation.DISABLE)) { - // it is not likely that instanceEnabled is unset, because when we set operation to disable, - // we always set instanceEnabled to false + // it is not likely that HELIX_ENABLED is unset, because when we set operation to disable, + // we always set HELIX_ENABLED to false // If instance is enabled by old version helix (not having instance operation), the instance config // will have HELIX_ENABLED set to true. In this case, we should override the instance operation to ENABLE if ("true".equals(_record.getSimpleField(InstanceConfigProperty.HELIX_ENABLED.name()))) { diff --git a/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java b/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java index 47ea88ac4d..0827ecd7f1 100644 --- a/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java +++ b/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java @@ -71,7 +71,22 @@ public void testSetInstanceEnableWithReason() { .get(InstanceConfig.InstanceConfigProperty.HELIX_DISABLED_REASON.toString()), reasonCode); Assert.assertEquals(instanceConfig.getInstanceDisabledReason(), reasonCode); Assert.assertEquals(instanceConfig.getInstanceDisabledType(), - InstanceConstants.InstanceDisabledType.USER_OPERATION.toString()); + InstanceConstants.InstanceDisabledType.USER_OPERATION.toString());} + + @Test + public void testEnabledInstanceBackwardCompatibility() { + // if instance is disabled by instanceOperation, and enabled by HELIX_ENABLED, the instance should be enabled + InstanceConfig instanceConfig = new InstanceConfig.Builder().setInstanceOperation(InstanceConstants.InstanceOperation.DISABLE) + .build("id"); + Assert.assertFalse(instanceConfig.getInstanceEnabled()); + // assume an old version client enabled the instance by setting HELIX_ENABLED to true + instanceConfig.getRecord().setSimpleField(InstanceConfig.InstanceConfigProperty.HELIX_ENABLED.name(), "true"); + Assert.assertTrue(instanceConfig.getInstanceEnabled()); + instanceConfig.setInstanceOperation(InstanceConstants.InstanceOperation.ENABLE); + + // disable the instance by setting HELIX_ENABLED to false + instanceConfig.getRecord().setSimpleField(InstanceConfig.InstanceConfigProperty.HELIX_ENABLED.name(), "false"); + Assert.assertFalse(instanceConfig.getInstanceEnabled()); } @Test From e338f3deac2e6bbd2e2d1dc95dc11ca3a7f541d9 Mon Sep 17 00:00:00 2001 From: xyuanlu Date: Mon, 2 Dec 2024 17:46:11 -0800 Subject: [PATCH 3/4] comments --- .../src/main/java/org/apache/helix/model/InstanceConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 508e3eeff2..a5ee410304 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 @@ -657,7 +657,7 @@ public InstanceOperation getInstanceOperation() { } // if instance operation id DISABLE, we override it to ENABLE if instance is enabled - if (activeInstanceOperation.getOperation().equals(InstanceConstants.InstanceOperation.DISABLE)) { + if (activeInstanceOperation.getOperation() == InstanceConstants.InstanceOperation.DISABLE) { // it is not likely that HELIX_ENABLED is unset, because when we set operation to disable, // we always set HELIX_ENABLED to false // If instance is enabled by old version helix (not having instance operation), the instance config From bdfab0547330878333b5d7c7441aa9e00b543a5b Mon Sep 17 00:00:00 2001 From: xyuanlu Date: Mon, 2 Dec 2024 20:35:24 -0800 Subject: [PATCH 4/4] edit typo in comment --- .../main/java/org/apache/helix/model/InstanceConfig.java | 2 +- .../java/org/apache/helix/model/TestInstanceConfig.java | 7 ++++--- 2 files changed, 5 insertions(+), 4 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 a5ee410304..7b5faddc34 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 @@ -656,7 +656,7 @@ public InstanceOperation getInstanceOperation() { .build(); } - // if instance operation id DISABLE, we override it to ENABLE if instance is enabled + // if instance operation is DISABLE, we override it to ENABLE if HELIX_ENABLED set to true if (activeInstanceOperation.getOperation() == InstanceConstants.InstanceOperation.DISABLE) { // it is not likely that HELIX_ENABLED is unset, because when we set operation to disable, // we always set HELIX_ENABLED to false diff --git a/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java b/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java index 0827ecd7f1..3645fa18eb 100644 --- a/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java +++ b/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java @@ -71,13 +71,14 @@ public void testSetInstanceEnableWithReason() { .get(InstanceConfig.InstanceConfigProperty.HELIX_DISABLED_REASON.toString()), reasonCode); Assert.assertEquals(instanceConfig.getInstanceDisabledReason(), reasonCode); Assert.assertEquals(instanceConfig.getInstanceDisabledType(), - InstanceConstants.InstanceDisabledType.USER_OPERATION.toString());} + InstanceConstants.InstanceDisabledType.USER_OPERATION.toString()); + } @Test public void testEnabledInstanceBackwardCompatibility() { // if instance is disabled by instanceOperation, and enabled by HELIX_ENABLED, the instance should be enabled - InstanceConfig instanceConfig = new InstanceConfig.Builder().setInstanceOperation(InstanceConstants.InstanceOperation.DISABLE) - .build("id"); + InstanceConfig instanceConfig = + new InstanceConfig.Builder().setInstanceOperation(InstanceConstants.InstanceOperation.DISABLE).build("id"); Assert.assertFalse(instanceConfig.getInstanceEnabled()); // assume an old version client enabled the instance by setting HELIX_ENABLED to true instanceConfig.getRecord().setSimpleField(InstanceConfig.InstanceConfigProperty.HELIX_ENABLED.name(), "true");