Re: [PR] Replace the HELIX_ENABLED config with InstanceOperation while maintaining backwards compatibility with old APIs [helix]

["zpinto (via GitHub)" <gi...@apache.org>]

zpinto commented on code in PR #2772:
URL: https://github.com/apache/helix/pull/2772#discussion_r1546796720


##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -321,29 +330,93 @@ public String getInstanceDisabledReason() {
    *         Default is am empty string.
    */
   public String getInstanceDisabledType() {
-    if (getInstanceEnabled()) {
+    if (!getInstanceOperation().equals(InstanceConstants.InstanceOperation.DISABLE)) {
       return InstanceConstants.INSTANCE_NOT_DISABLED;
     }
     return _record.getStringField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(),
         InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name());
   }
 
   /**
-   * Get the timestamp (milliseconds from epoch) when this instance was enabled/disabled last time.
+   * Set the instance operation for this instance.
    *
-   * @return
+   * @param operation the instance operation
    */
-  public long getInstanceEnabledTime() {
-    return _record.getLongField(InstanceConfigProperty.HELIX_ENABLED_TIMESTAMP.name(), -1);
-  }
-
   public void setInstanceOperation(InstanceConstants.InstanceOperation operation) {
     _record.setSimpleField(InstanceConfigProperty.INSTANCE_OPERATION.name(),
         operation == null ? "" : operation.name());
+    if (operation == null || operation == InstanceConstants.InstanceOperation.ENABLE
+        || operation == InstanceConstants.InstanceOperation.DISABLE) {
+      // We are still setting the HELIX_ENABLED field for backwards compatibility.
+      // It is possible that users will be using earlier version of HelixAdmin or helix-rest
+      // is on older version.
+      // TODO: Remove this when we are sure that all users are using the new field INSTANCE_OPERATION.
+      setInstanceEnabledHelper(!(operation == InstanceConstants.InstanceOperation.DISABLE));
+    }
+  }
+
+  private void setInstanceOperationInit(InstanceConstants.InstanceOperation operation) {
+    if (operation == null) {
+      return;
+    }
+    _record.setSimpleField(InstanceConfigProperty.INSTANCE_OPERATION.name(), operation.name());
+  }
+
+  /**
+   * Get the InstanceOperation of this instance, default is ENABLE if nothing is set. If
+   * HELIX_ENABLED is set to false, then the instance operation is DISABLE for backwards
+   * compatibility.
+   *
+   * @return the instance operation
+   */
+  public InstanceConstants.InstanceOperation getInstanceOperation() {
+    String instanceOperationString =
+        _record.getSimpleField(InstanceConfigProperty.INSTANCE_OPERATION.name());
+
+    InstanceConstants.InstanceOperation instanceOperation;
+    try {
+      // If INSTANCE_OPERATION is not set, then the instance is enabled.
+      instanceOperation = instanceOperationString == null || instanceOperationString.isEmpty()
+          ? InstanceConstants.InstanceOperation.ENABLE
+          : InstanceConstants.InstanceOperation.valueOf(instanceOperationString);
+    } catch (IllegalArgumentException e) {
+      _logger.error("Invalid instance operation: " + instanceOperationString + " for instance: "
+          + _record.getId()
+          + ". You may need to update your version of Helix to get support for this "
+          + "type of InstanceOperation. Defaulting to UNKNOWN.");
+      return InstanceConstants.InstanceOperation.UNKNOWN;
+    }
+
+    // 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(
+        instanceOperation))) {
+      return InstanceConstants.InstanceOperation.DISABLE;
+    }
+
+    return instanceOperation;
   }
 
-  public String getInstanceOperation() {
-    return _record.getStringField(InstanceConfigProperty.INSTANCE_OPERATION.name(), "");
+  /**
+   * Check if this instance is enabled. This is used to determine if the instance can host online
+   * replicas and take new assignment.
+   *
+   * @return true if enabled, false otherwise
+   */
+  public boolean getInstanceEnabled() {

Review Comment:
   This API is backwards compatible because getInstanceOperation will translate the old HELIX_ENABLED field to the new InstanceOperation field.



-- 
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: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org