From bde8ff89d2ef6fa7ff7d188c1eeae5f9a4bc32a5 Mon Sep 17 00:00:00 2001
From: xyuanlu <xyuanlu@gmail.com>
Date: Thu, 5 Dec 2024 14:40:56 -0800
Subject: [PATCH] Fix IsInstanceEnabled for backward compatibility (#2972)

Fix IsInstanceEnabled for backward compatibility
---
 .../org/apache/helix/model/InstanceConfig.java   | 15 +++++++++++++--
 .../apache/helix/model/TestInstanceConfig.java   | 16 ++++++++++++++++
 2 files changed, 29 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..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
@@ -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 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
+      // 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;
   }
 
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..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
@@ -74,6 +74,22 @@ public void testSetInstanceEnableWithReason() {
         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
   public void testGetParsedDomainEmptyDomain() {
     InstanceConfig instanceConfig = new InstanceConfig(new ZNRecord("id"));