Fix IsInstanceEnabled for backward compatibility (apache#2972)

Fix IsInstanceEnabled for backward compatibility

Author: xyuanlu

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;
   }
 

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"));