[GitHub] [helix] junkaixue commented on a change in pull request #1993: Add instance disable reason

[GitBox <gi...@apache.org>]

junkaixue commented on a change in pull request #1993:
URL: https://github.com/apache/helix/pull/1993#discussion_r833804482



##########
File path: helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
##########
@@ -275,13 +278,55 @@ public boolean getInstanceEnabled() {
 
   /**
    * Set the enabled state of the instance
+   * If user enables the instance, HELIX_DISABLED_REASON filed will be removed.
    *
    * @param enabled true to enable, false to disable
    */
   public void setInstanceEnabled(boolean enabled) {
     _record.setBooleanField(InstanceConfigProperty.HELIX_ENABLED.toString(), enabled);
     _record.setLongField(InstanceConfigProperty.HELIX_ENABLED_TIMESTAMP.name(),
         System.currentTimeMillis());
+    if (enabled) {
+      _record.getSimpleFields().remove(InstanceConfigProperty.HELIX_DISABLED_REASON.toString());
+      _record.getSimpleFields().remove(InstanceConfigProperty.HELIX_DISABLED_TYPE.toString());
+    }
+  }
+
+  /**
+   * Set the instance disabled reason when instance is disabled.
+   * It will be a no-op when instance is enabled.
+   */
+  public void setInstanceDisabledReason(String disabledReason) {
+     if (!getInstanceEnabled()) {
+     _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_REASON.toString(), disabledReason);
+     }
+  }
+
+  /**
+   * Set the instance disabled type when instance is disabled.
+   * It will be a no-op when instance is enabled.
+   */
+  public void setInstanceDisabledType(InstanceConstants.InstanceDisabledType disabledType) {
+    if (!getInstanceEnabled()) {
+      _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_TYPE.toString(),
+          disabledType.toString());
+    }
+  }
+
+  /**
+   * @return Return instance disabled reason. Default is am empty string.
+   */
+  public String getInstanceDisabledReason() {
+    return _record.getStringField(InstanceConfigProperty.HELIX_DISABLED_REASON.toString(), "");
+  }
+
+  /**
+   *
+   * @return Return instance disabled type (org.apache.helix.constants.InstanceConstants.InstanceDisabledType)
+   *         Default is am empty string.
+   */
+  public String getInstanceDisabledType() {
+    return _record.getStringField(InstanceConfigProperty.HELIX_DISABLED_TYPE.toString(), "");

Review comment:
       Put the default type as default value.

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1876,6 +1885,14 @@ public ZNRecord update(ZNRecord currentData) {
 
         InstanceConfig config = new InstanceConfig(currentData);
         config.setInstanceEnabled(enabled);
+        if (!enabled) {
+          if (reason!=null) {
+            config.setInstanceDisabledReason(reason);
+          }
+          if (disabledType != null) {
+            config.setInstanceDisabledType(disabledType);
+          }

Review comment:
       Let's have a default type for it. Then it can support backward compatible.




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