junkaixue commented on code in PR #2801: URL: https://github.com/apache/helix/pull/2801#discussion_r1628113175
########## helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java: ########## @@ -24,15 +24,52 @@ public class InstanceConstants { /** * The set of InstanceOperations that are not allowed to be populated in the RoutingTableProvider. */ - public static final Set<InstanceOperation> UNSERVABLE_INSTANCE_OPERATIONS = + public static final Set<InstanceOperation> UNROUTABLE_INSTANCE_OPERATIONS = Set.of(InstanceOperation.SWAP_IN, InstanceOperation.UNKNOWN); + @Deprecated public enum InstanceDisabledType { CLOUD_EVENT, USER_OPERATION, DEFAULT_INSTANCE_DISABLE_TYPE } + public enum InstanceOperationSource { + AUTOMATION, ADMIN, USER; Review Comment: Have integer property for its priority? Say ADMIN(0), USER(1) and AUTOMATION(2)? ########## helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java: ########## @@ -24,15 +24,52 @@ public class InstanceConstants { /** * The set of InstanceOperations that are not allowed to be populated in the RoutingTableProvider. */ - public static final Set<InstanceOperation> UNSERVABLE_INSTANCE_OPERATIONS = + public static final Set<InstanceOperation> UNROUTABLE_INSTANCE_OPERATIONS = Set.of(InstanceOperation.SWAP_IN, InstanceOperation.UNKNOWN); + @Deprecated public enum InstanceDisabledType { CLOUD_EVENT, USER_OPERATION, DEFAULT_INSTANCE_DISABLE_TYPE } + public enum InstanceOperationSource { + AUTOMATION, ADMIN, USER; + + /** + * Convert from InstanceOperationTrigger to DisabledType + * + * @param trigger InstanceOperationTrigger + * @return InstanceDisabledType + */ + public static InstanceDisabledType instanceOperationSourceToInstanceDisabledType( Review Comment: Let's remove this type concept. It is redundant and could be confusing. If we already defined the AUTOMATION. The source is AUTOMATION then it does not have to be CLOUD_EVENT. User can categorize it with any types. Don't blindly bundles these two together. ########## helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java: ########## @@ -328,75 +462,192 @@ public String getInstanceDisabledReason() { * * @return Return instance disabled type (org.apache.helix.constants.InstanceConstants.InstanceDisabledType) * Default is am empty string. + * @deprecated This method is deprecated. Please use getInstanceOperation().getSource + *() instead. */ + @Deprecated public String getInstanceDisabledType() { - if (!getInstanceOperation().equals(InstanceConstants.InstanceOperation.DISABLE)) { + if (_record.getBooleanField(InstanceConfigProperty.HELIX_ENABLED.name(), + HELIX_ENABLED_DEFAULT_VALUE)) { return InstanceConstants.INSTANCE_NOT_DISABLED; } return _record.getStringField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(), InstanceConstants.InstanceDisabledType.DEFAULT_INSTANCE_DISABLE_TYPE.name()); } + private List<InstanceOperation> getInstanceOperations() { + if (_deserializedInstanceOperations == null || _deserializedInstanceOperations.isEmpty()) { + // If the _deserializedInstanceOperationMap is not set, then we need to build it from the real + // helix property HELIX_INSTANCE_OPERATION_MAP. + List<String> instanceOperations = + _record.getListField(InstanceConfigProperty.HELIX_INSTANCE_OPERATIONS.name()); + List<InstanceOperation> newDeserializedInstanceOperations = new ArrayList<>(); + if (instanceOperations != null) { + for (String serializedInstanceOperation : instanceOperations) { + try { + Map<String, String> properties = _objectMapper.readValue(serializedInstanceOperation, + new TypeReference<Map<String, String>>() { + }); + newDeserializedInstanceOperations.add(new InstanceOperation(properties)); + } catch (JsonProcessingException e) { + _logger.error( + "Failed to deserialize instance operation for instance: " + _record.getId(), e); + } + } + } + _deserializedInstanceOperations = newDeserializedInstanceOperations; + } + + return _deserializedInstanceOperations; + } + /** * 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(InstanceConstants.InstanceOperation operation) { - _record.setSimpleField(InstanceConfigProperty.INSTANCE_OPERATION.name(), - operation == null ? "" : operation.name()); - if (operation == null || operation == InstanceConstants.InstanceOperation.ENABLE - || operation == InstanceConstants.InstanceOperation.DISABLE) { + public void setInstanceOperation(InstanceOperation operation) { + List<InstanceOperation> deserializedInstanceOperations = getInstanceOperations(); + + if (operation.getSource() == InstanceConstants.InstanceOperationSource.ADMIN) { + deserializedInstanceOperations.clear(); + } else { + // Remove the instance operation with the same trigger if it exists. + deserializedInstanceOperations.removeIf( + instanceOperation -> instanceOperation.getSource() == operation.getSource()); + } + if (operation.getOperation() != InstanceConstants.InstanceOperation.ENABLE) { + deserializedInstanceOperations.add(operation); + } + // Set the actual field in the ZnRecord + _record.setListField(InstanceConfigProperty.HELIX_INSTANCE_OPERATIONS.name(), + deserializedInstanceOperations.stream().map(instanceOperation -> { + try { + return _objectMapper.writeValueAsString(instanceOperation.getProperties()); + } catch (JsonProcessingException e) { + throw new HelixException( + "Failed to serialize instance operation for instance: " + _record.getId() + + " Can't set the instance operation to: " + operation.getOperation(), e); + } + }).collect(Collectors.toList())); + + // TODO: Remove this when we are sure that all users are using the new InstanceOperation only and HELIX_ENABLED is removed. + if (operation.getOperation() == 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)); + + if (_record.getBooleanField(InstanceConfigProperty.HELIX_ENABLED.name(), true)) { + // Check if it is already disabled, if yes, then we don't need to set HELIX_ENABLED and HELIX_ENABLED_TIMESTAMP + setInstanceEnabledHelper(false, operation.getTimestamp()); + } + + _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_REASON.name(), + operation.getReason()); + _record.setSimpleField(InstanceConfigProperty.HELIX_DISABLED_TYPE.name(), + InstanceConstants.InstanceOperationSource.instanceOperationSourceToInstanceDisabledType( + operation.getSource()).name()); + } else if (operation.getOperation() == InstanceConstants.InstanceOperation.ENABLE) { Review Comment: We need to think about the backward compatible behavior. Say the instance was disabled in old field, but no new instance operations. Then we should let it be disabled. My suggestion for this implementation to get old field disable time and merge in the new operation stack. Then we can populate it right and not have these branches. ########## helix-common/src/main/java/org/apache/helix/constants/InstanceConstants.java: ########## @@ -24,15 +24,52 @@ public class InstanceConstants { /** * The set of InstanceOperations that are not allowed to be populated in the RoutingTableProvider. */ - public static final Set<InstanceOperation> UNSERVABLE_INSTANCE_OPERATIONS = + public static final Set<InstanceOperation> UNROUTABLE_INSTANCE_OPERATIONS = Set.of(InstanceOperation.SWAP_IN, InstanceOperation.UNKNOWN); + @Deprecated public enum InstanceDisabledType { CLOUD_EVENT, USER_OPERATION, DEFAULT_INSTANCE_DISABLE_TYPE } + public enum InstanceOperationSource { + AUTOMATION, ADMIN, USER; + + /** + * Convert from InstanceOperationTrigger to DisabledType + * + * @param trigger InstanceOperationTrigger + * @return InstanceDisabledType + */ + public static InstanceDisabledType instanceOperationSourceToInstanceDisabledType( Review Comment: If we would like to offer this categorization, let's just give an entry that user can fill any type there. This type translation I would assume the applications define in their code. ########## helix-core/src/main/java/org/apache/helix/cloud/event/helix/DefaultCloudEventCallbackImpl.java: ########## @@ -72,10 +80,14 @@ public void enableInstance(HelixManager manager, Object eventInfo) { HelixEventHandlingUtil .updateCloudEventOperationInClusterConfig(manager.getClusterName(), instanceName, manager.getHelixDataAccessor().getBaseDataAccessor(), true, message); - if (HelixEventHandlingUtil.isInstanceDisabledForCloudEvent(instanceName, accessor)) { - manager.getClusterManagmentTool().enableInstance(manager.getClusterName(), instanceName, true, - InstanceConstants.InstanceDisabledType.CLOUD_EVENT, message); - } + InstanceUtil instanceUtil = new InstanceUtil(manager.getConfigAccessor(), Review Comment: Util usually are static? ########## helix-core/src/main/java/org/apache/helix/util/InstanceUtil.java: ########## @@ -0,0 +1,176 @@ +package org.apache.helix.util; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import java.util.List; +import java.util.stream.Collectors; + +import com.google.common.collect.ImmutableSet; +import org.apache.helix.AccessOption; +import org.apache.helix.BaseDataAccessor; +import org.apache.helix.ConfigAccessor; +import org.apache.helix.HelixException; +import org.apache.helix.PropertyPathBuilder; +import org.apache.helix.constants.InstanceConstants; +import org.apache.helix.manager.zk.ZKHelixDataAccessor; +import org.apache.helix.manager.zk.ZkBaseDataAccessor; +import org.apache.helix.model.ClusterTopologyConfig; +import org.apache.helix.model.HelixConfigScope; +import org.apache.helix.model.InstanceConfig; +import org.apache.helix.model.builder.HelixConfigScopeBuilder; +import org.apache.helix.zookeeper.api.client.RealmAwareZkClient; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.apache.helix.zookeeper.zkclient.DataUpdater; + +public class InstanceUtil { + private final BaseDataAccessor<ZNRecord> _baseDataAccessor; + private final ConfigAccessor _configAccessor; + + public InstanceUtil(RealmAwareZkClient zkClient) { + _baseDataAccessor = new ZkBaseDataAccessor<>(zkClient); + _configAccessor = new ConfigAccessor(zkClient); + } Review Comment: As Util, it should be static. Have a client bundled here will be risky. If user already close the client and use a new client, then this class should be destroyed, not efficient. This is why usually util is static methods and it can take zkclient as one argument as needed. ########## helix-core/src/main/java/org/apache/helix/util/InstanceUtil.java: ########## @@ -0,0 +1,176 @@ +package org.apache.helix.util; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import java.util.List; +import java.util.stream.Collectors; + +import com.google.common.collect.ImmutableSet; +import org.apache.helix.AccessOption; +import org.apache.helix.BaseDataAccessor; +import org.apache.helix.ConfigAccessor; +import org.apache.helix.HelixException; +import org.apache.helix.PropertyPathBuilder; +import org.apache.helix.constants.InstanceConstants; +import org.apache.helix.manager.zk.ZKHelixDataAccessor; +import org.apache.helix.manager.zk.ZkBaseDataAccessor; +import org.apache.helix.model.ClusterTopologyConfig; +import org.apache.helix.model.HelixConfigScope; +import org.apache.helix.model.InstanceConfig; +import org.apache.helix.model.builder.HelixConfigScopeBuilder; +import org.apache.helix.zookeeper.api.client.RealmAwareZkClient; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.apache.helix.zookeeper.zkclient.DataUpdater; + +public class InstanceUtil { + private final BaseDataAccessor<ZNRecord> _baseDataAccessor; + private final ConfigAccessor _configAccessor; + + public InstanceUtil(RealmAwareZkClient zkClient) { + _baseDataAccessor = new ZkBaseDataAccessor<>(zkClient); + _configAccessor = new ConfigAccessor(zkClient); + } + + public InstanceUtil(ConfigAccessor configAccessor, BaseDataAccessor<ZNRecord> baseAccessor) { + _baseDataAccessor = baseAccessor; + _configAccessor = configAccessor; + } + + public static void validateInstanceOperationTransition(InstanceConfig matchingLogicalIdInstance, Review Comment: Why not build a static map like: ENABLE -> Set(DISABLE, EVACUTATE) DISABLE. -> Set(....) So the logic will just be map.get(currentOperation).contains(futureOperation). -- 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