xyuanlu commented on code in PR #2987:
URL: https://github.com/apache/helix/pull/2987#discussion_r1920888609


##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -555,31 +574,68 @@ public void setInstanceOperation(InstanceOperation 
operation) {
         setInstanceEnabledHelper(false, operation.getTimestamp());
       }
 
-      
_record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_REASON.name(),
-          operation.getReason());
-      _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(),
-          
InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name());
+      resetInstanceDisabledTypeAndReason();
+      setInstanceDisabledReason(operation.getReason());
+      setInstanceDisabledType(operation.getLegacyDisabledType());
     } else if (operation.getOperation() == 
InstanceConstants.InstanceOperation.ENABLE) {
-      // If any of the other InstanceOperations are of type DISABLE, set that 
in the HELIX_ENABLED,
-      // HELIX_DISABLED_REASON, and HELIX_DISABLED_TYPE fields.
-      InstanceOperation latestDisableInstanceOperation = null;
-      for (InstanceOperation instanceOperation : getInstanceOperations()) {
-        if (instanceOperation.getOperation() == 
InstanceConstants.InstanceOperation.DISABLE && (
-            latestDisableInstanceOperation == null || 
instanceOperation.getTimestamp()
-                > latestDisableInstanceOperation.getTimestamp())) {
-          latestDisableInstanceOperation = instanceOperation;
+      // Ensure HELIX_ENABLED reflects the latest disable operation if 
applicable.
+      InstanceOperation latestDisableInstanceOperation = 
getInstanceOperations().stream()
+          .filter(op -> op.getOperation() == 
InstanceConstants.InstanceOperation.DISABLE)
+          
.max(Comparator.comparingLong(InstanceOperation::getTimestamp)).orElse(null);
+
+      if (!_record.getBooleanField(InstanceConfigProperty.HELIX_ENABLED.name(),
+          HELIX_ENABLED_DEFAULT_VALUE)) {
+        // Only set the disable reason and disable type if the HELIX_ENABLED 
is false because HELIX_ENABLED
+        // being true takes precedence over an existing latest disable 
operation existing.
+        if (latestDisableInstanceOperation != null) {
+          
setInstanceDisabledReason(latestDisableInstanceOperation.getReason());
+          
setInstanceDisabledType(latestDisableInstanceOperation.getLegacyDisabledType());
+        } else {
+          setInstanceEnabledHelper(true, operation.getTimestamp());
         }
       }
+    }
+  }
 
-      if (latestDisableInstanceOperation != null) {
-        
_record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_REASON.name(),
-            latestDisableInstanceOperation.getReason());
-        
_record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(),
-            
InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name());
-      } else {
-        setInstanceEnabledHelper(true, operation.getTimestamp());
-      }
+  private void setInstanceOperationHelper(InstanceOperation operation) {
+    List<InstanceOperation> operations = getInstanceOperations();
+
+    if (operation.getSource() == 
InstanceConstants.InstanceOperationSource.ADMIN) {
+      operations.clear();
+    } else {
+      // Remove existing operation with the same source.
+      operations.removeIf(op -> op.getSource() == operation.getSource());
+    }
+
+    insertInstanceOperation(operations, operation);
+
+    // Serialize and update ZnRecord.
+    
_record.setListField(InstanceConfigProperty.HELIX_INSTANCE_OPERATIONS.name(),
+        serializeInstanceOperations(operations));
+
+    setLegacyFieldsForInstanceOperation(operation);
+  }
+
+  /**
+   * Set the instance operation for this instance. This method also sets the 
HELIX_ENABLED,
+   * HELIX_DISABLED_REASON, and HELIX_DISABLED_TYPE fields for backwards 
compatibility.
+   *
+   * @param operation the instance operation
+   */
+  public void setInstanceOperation(InstanceOperation operation) {
+    // TODO: This can be removed when HELIX_ENABLED is removed.
+    // This is used for backwards compatibility. getInstanceOperation will 
respect the old
+    // legacy field of HELIX_ENABLED which could have been set by older 
versions of Helix.
+    // If the HELIX_ENABLED is set by an older version of Helix, we need to 
sync that value to the
+    // new InstanceOperation field when any new instance operation is set.
+    // We only need to do this if there is already an instance operation set. 
If there isn't, then
+    // the default is ENABLE with a source of DEFAULT.
+    if (!getInstanceOperations().isEmpty()) {
+      setInstanceOperationHelper(getInstanceOperation());

Review Comment:
   Do you think we could move the write back logic inside 
`setInstanceOperationHelper `?
   Now if Helix_enabled is set, we call `setInstanceOperationHelper ` twice and 
in each call we also call `setLegacyFieldsForInstanceOperation`. 
   I think in the first `setInstanceOperationHelper`, probably there is no need 
to overwrite helix_enabled again.



-- 
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-unsubscr...@helix.apache.org

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


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

Reply via email to