This is an automated email from the ASF dual-hosted git repository.
hulee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git
The following commit(s) were added to refs/heads/master by this push:
new 30fc9cc70 Code refactor and cleanup on instance validation (#2032)
30fc9cc70 is described below
commit 30fc9cc70bfc56ff0fbcb59d787d3a06d277bfc1
Author: Qi (Quincy) Qu <[email protected]>
AuthorDate: Mon Apr 18 10:57:15 2022 -0700
Code refactor and cleanup on instance validation (#2032)
Unify the usage of checking instance enable/disable using
InstanceValidationUtil
---
.../dataproviders/BaseControllerDataProvider.java | 16 ++++--------
.../controller/rebalancer/topology/Topology.java | 9 ++-----
.../rebalancer/util/DelayedRebalanceUtil.java | 13 ++++------
.../controller/stages/ReadClusterDataStage.java | 4 +--
.../org/apache/helix/manager/zk/ZKHelixAdmin.java | 5 +---
.../java/org/apache/helix/model/ClusterConfig.java | 5 ++--
.../java/org/apache/helix/tools/ClusterSetup.java | 4 +--
.../main/java/org/apache/helix/util/HelixUtil.java | 7 ++---
.../apache/helix/util/InstanceValidationUtil.java | 30 +++++++++++++++++-----
.../helix/util/TestInstanceValidationUtil.java | 6 ++---
.../MaintenanceManagementService.java | 2 +-
.../server/resources/helix/InstancesAccessor.java | 4 +--
12 files changed, 54 insertions(+), 51 deletions(-)
diff --git
a/helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
b/helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
index 3c705f4b9..70acf31ad 100644
---
a/helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
+++
b/helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
@@ -62,6 +62,7 @@ import org.apache.helix.model.ResourceConfig;
import org.apache.helix.model.StateModelDefinition;
import org.apache.helix.task.TaskConstants;
import org.apache.helix.util.HelixUtil;
+import org.apache.helix.util.InstanceValidationUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -798,25 +799,18 @@ public class BaseControllerDataProvider implements
ControlContextProvider {
_disabledInstanceSet.clear();
for (InstanceConfig config : instanceConfigs) {
Map<String, List<String>> disabledPartitionMap =
config.getDisabledPartitionsMap();
- if (!config.getInstanceEnabled()) {
+ if (!InstanceValidationUtil.isInstanceEnabled(config, clusterConfig)) {
_disabledInstanceSet.add(config.getInstanceName());
}
for (String resource : disabledPartitionMap.keySet()) {
- if (!_disabledInstanceForPartitionMap.containsKey(resource)) {
- _disabledInstanceForPartitionMap.put(resource, new HashMap<>());
- }
+ _disabledInstanceForPartitionMap.putIfAbsent(resource, new
HashMap<>());
for (String partition : disabledPartitionMap.get(resource)) {
- if
(!_disabledInstanceForPartitionMap.get(resource).containsKey(partition)) {
- _disabledInstanceForPartitionMap.get(resource).put(partition, new
HashSet<>());
- }
- _disabledInstanceForPartitionMap.get(resource).get(partition)
+ _disabledInstanceForPartitionMap.get(resource)
+ .computeIfAbsent(partition, key -> new HashSet<>())
.add(config.getInstanceName());
}
}
}
- if (clusterConfig != null && clusterConfig.getDisabledInstances() != null)
{
-
_disabledInstanceSet.addAll(clusterConfig.getDisabledInstances().keySet());
- }
}
/*
diff --git
a/helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
b/helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
index 98ef0c598..dba120168 100644
---
a/helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
+++
b/helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
@@ -32,6 +32,7 @@ import org.apache.helix.HelixException;
import org.apache.helix.model.ClusterConfig;
import org.apache.helix.model.ClusterTopologyConfig;
import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.util.InstanceValidationUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -168,7 +169,7 @@ public class Topology {
}
addEndNode(root, instanceName, instanceTopologyMap, weight,
_liveInstances);
} catch (IllegalArgumentException e) {
- if (isInstanceEnabled(clusterConfig, instanceName, insConfig)) {
+ if (InstanceValidationUtil.isInstanceEnabled(insConfig,
clusterConfig)) {
throw e;
} else {
logger.warn("Topology setting {} for instance {} is unset or
invalid, ignore the instance!",
@@ -179,12 +180,6 @@ public class Topology {
return root;
}
- private static boolean isInstanceEnabled(ClusterConfig clusterConfig, String
instanceName,
- InstanceConfig instanceConfig) {
- return (instanceConfig.getInstanceEnabled() &&
(clusterConfig.getDisabledInstances() == null
- || !clusterConfig.getDisabledInstances().containsKey(instanceName)));
- }
-
/**
* Construct the instance topology map for an instance.
* The mapping is the cluster topology path name to its corresponding value.
diff --git
a/helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java
b/helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java
index 43fcf4c5a..41df15005 100644
---
a/helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java
+++
b/helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java
@@ -31,7 +31,7 @@ import org.apache.helix.model.ClusterConfig;
import org.apache.helix.model.IdealState;
import org.apache.helix.model.InstanceConfig;
import org.apache.helix.model.ResourceConfig;
-import org.apache.helix.util.ConfigStringUtil;
+import org.apache.helix.util.InstanceValidationUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -137,15 +137,12 @@ public class DelayedRebalanceUtil {
}
// check the time instance got disabled.
- if (!instanceConfig.getInstanceEnabled() ||
(clusterConfig.getDisabledInstances() != null
- && clusterConfig.getDisabledInstances().containsKey(instance))) {
+ if (!InstanceValidationUtil.isInstanceEnabled(instanceConfig,
clusterConfig)) {
long disabledTime = instanceConfig.getInstanceEnabledTime();
- if (clusterConfig.getDisabledInstances() != null &&
clusterConfig.getDisabledInstances()
- .containsKey(instance)) {
+ Map<String, String> disabledInstances =
clusterConfig.getDisabledInstances();
+ if (disabledInstances.containsKey(instance)) {
// Update batch disable time
- long batchDisableTime = Long.parseLong(ConfigStringUtil
-
.parseConcatenatedConfig(clusterConfig.getDisabledInstances().get(instance))
-
.get(ClusterConfig.ClusterConfigProperty.HELIX_ENABLED_DISABLE_TIMESTAMP.toString()));
+ long batchDisableTime =
Long.parseLong(clusterConfig.getInstanceHelixDisabledTimeStamp(instance));
if (disabledTime == -1 || disabledTime > batchDisableTime) {
disabledTime = batchDisableTime;
}
diff --git
a/helix-core/src/main/java/org/apache/helix/controller/stages/ReadClusterDataStage.java
b/helix-core/src/main/java/org/apache/helix/controller/stages/ReadClusterDataStage.java
index f17de53a4..a7e9742e0 100644
---
a/helix-core/src/main/java/org/apache/helix/controller/stages/ReadClusterDataStage.java
+++
b/helix-core/src/main/java/org/apache/helix/controller/stages/ReadClusterDataStage.java
@@ -41,6 +41,7 @@ import org.apache.helix.model.InstanceConfig;
import org.apache.helix.model.LiveInstance;
import org.apache.helix.model.Message;
import org.apache.helix.monitoring.mbeans.ClusterStatusMonitor;
+import org.apache.helix.util.InstanceValidationUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -91,8 +92,7 @@ public class ReadClusterDataStage extends AbstractBaseStage {
instanceMessageMap.put(instanceName,
Sets.newHashSet(dataProvider.getMessages(instanceName).values()));
}
- if (!config.getInstanceEnabled() ||
(clusterConfig.getDisabledInstances() != null
- &&
clusterConfig.getDisabledInstances().containsKey(instanceName))) {
+ if (!InstanceValidationUtil.isInstanceEnabled(config,
clusterConfig)) {
disabledInstanceSet.add(instanceName);
}
diff --git
a/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
b/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
index 0d05d05b2..28a9e4bcc 100644
--- a/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
+++ b/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
@@ -1928,10 +1928,7 @@ public class ZKHelixAdmin implements HelixAdmin {
}
ClusterConfig clusterConfig = new ClusterConfig(currentData);
- Map<String, String> disabledInstances = new TreeMap<>();
- if (clusterConfig.getDisabledInstances() != null) {
- disabledInstances.putAll(clusterConfig.getDisabledInstances());
- }
+ Map<String, String> disabledInstances = new
TreeMap<>(clusterConfig.getDisabledInstances());
if (enabled) {
disabledInstances.keySet().removeAll(instances);
} else {
diff --git a/helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
b/helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
index e279856f4..485076ee4 100644
--- a/helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
+++ b/helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
@@ -774,10 +774,11 @@ public class ClusterConfig extends HelixProperty {
/**
* Get current disabled instance map of <instance, disabledTimeStamp>
- * @return
+ * @return a non-null map of disabled instances in cluster config
*/
public Map<String, String> getDisabledInstances() {
- return
_record.getMapField(ClusterConfigProperty.DISABLED_INSTANCES.name());
+ Map<String, String> disabledInstances =
_record.getMapField(ClusterConfigProperty.DISABLED_INSTANCES.name());
+ return disabledInstances == null ? Collections.emptyMap() :
disabledInstances;
}
/**
diff --git a/helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
b/helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
index ab236e040..e01828c28 100644
--- a/helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
+++ b/helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
@@ -68,6 +68,7 @@ import org.apache.helix.model.builder.ConstraintItemBuilder;
import org.apache.helix.model.builder.HelixConfigScopeBuilder;
import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
import org.apache.helix.util.HelixUtil;
+import org.apache.helix.util.InstanceValidationUtil;
import org.apache.helix.zookeeper.api.client.HelixZkClient;
import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.apache.helix.zookeeper.datamodel.ZNRecord;
@@ -308,8 +309,7 @@ public class ClusterSetup {
ClusterConfig clusterConfig =
accessor.getProperty(keyBuilder.clusterConfig());
// ensure node is disabled, otherwise fail
- if (config.getInstanceEnabled() && (clusterConfig.getDisabledInstances()
== null
- || !clusterConfig.getDisabledInstances().containsKey(instanceId))) {
+ if (InstanceValidationUtil.isInstanceEnabled(config, clusterConfig)) {
String error = "Node " + instanceId + " is enabled, cannot drop";
_logger.warn(error);
throw new HelixException(error);
diff --git a/helix-core/src/main/java/org/apache/helix/util/HelixUtil.java
b/helix-core/src/main/java/org/apache/helix/util/HelixUtil.java
index ee31e4319..ad4b6931d 100644
--- a/helix-core/src/main/java/org/apache/helix/util/HelixUtil.java
+++ b/helix-core/src/main/java/org/apache/helix/util/HelixUtil.java
@@ -398,9 +398,10 @@ public final class HelixUtil {
idealState.getMaxPartitionsPerInstance());
// Remove all disabled instances so that Helix will not consider them live.
- List<String> disabledInstance =
- instanceConfigs.stream().filter(enabled ->
!enabled.getInstanceEnabled())
- .map(InstanceConfig::getInstanceName).collect(Collectors.toList());
+ List<String> disabledInstance = instanceConfigs.stream()
+ .filter(instanceConfig ->
!InstanceValidationUtil.isInstanceEnabled(instanceConfig, clusterConfig))
+ .map(InstanceConfig::getInstanceName)
+ .collect(Collectors.toList());
liveInstances.removeAll(disabledInstance);
Map<String, List<String>> preferenceLists = strategy
diff --git
a/helix-core/src/main/java/org/apache/helix/util/InstanceValidationUtil.java
b/helix-core/src/main/java/org/apache/helix/util/InstanceValidationUtil.java
index 48ff86b95..6f17d1c49 100644
--- a/helix-core/src/main/java/org/apache/helix/util/InstanceValidationUtil.java
+++ b/helix-core/src/main/java/org/apache/helix/util/InstanceValidationUtil.java
@@ -114,11 +114,22 @@ public class InstanceValidationUtil {
: instanceConfig.getInstanceDisabledType();
}
- private static boolean isInstanceEnabled(InstanceConfig instanceConfig,
ClusterConfig clusterConfig) {
+ /**
+ * Check if the instance is enabled by configuration
+ * @param instanceConfig
+ * @param clusterConfig
+ * @return
+ */
+ public static boolean isInstanceEnabled(InstanceConfig instanceConfig,
ClusterConfig clusterConfig) {
+ if (instanceConfig == null) {
+ throw new HelixException("InstanceConfig is NULL");
+ }
boolean enabledInInstanceConfig = instanceConfig.getInstanceEnabled();
- Map<String, String> disabledInstances =
clusterConfig.getDisabledInstances();
+ if (clusterConfig == null) {
+ return enabledInInstanceConfig;
+ }
boolean enabledInClusterConfig =
- disabledInstances == null ||
!disabledInstances.keySet().contains(instanceConfig.getInstanceName());
+
!clusterConfig.getDisabledInstances().containsKey(instanceConfig.getInstanceName());
return enabledInClusterConfig && enabledInInstanceConfig;
}
@@ -134,16 +145,23 @@ public class InstanceValidationUtil {
return liveInstance != null;
}
+ /**
+ * Deprecated. Please use {@link #isResourceAssigned} instead.
+ */
+ @Deprecated
+ public static boolean hasResourceAssigned(HelixDataAccessor dataAccessor,
String clusterId,
+ String instanceName) {
+ return isResourceAssigned(dataAccessor, instanceName);
+ }
+
/**
* Method to check if the instance is assigned at least 1 resource, not in a
idle state;
* Independent of the instance alive/enabled status
* @param dataAccessor
- * @param clusterId
* @param instanceName
* @return
*/
- public static boolean hasResourceAssigned(HelixDataAccessor dataAccessor,
String clusterId,
- String instanceName) {
+ public static boolean isResourceAssigned(HelixDataAccessor dataAccessor,
String instanceName) {
PropertyKey.Builder propertyKeyBuilder = dataAccessor.keyBuilder();
LiveInstance liveInstance =
dataAccessor.getProperty(propertyKeyBuilder.liveInstance(instanceName));
if (liveInstance != null) {
diff --git
a/helix-core/src/test/java/org/apache/helix/util/TestInstanceValidationUtil.java
b/helix-core/src/test/java/org/apache/helix/util/TestInstanceValidationUtil.java
index 04c110dcf..fe04d5e5e 100644
---
a/helix-core/src/test/java/org/apache/helix/util/TestInstanceValidationUtil.java
+++
b/helix-core/src/test/java/org/apache/helix/util/TestInstanceValidationUtil.java
@@ -136,7 +136,7 @@ public class TestInstanceValidationUtil {
.getProperty(argThat(new
PropertyKeyArgument(PropertyType.CURRENTSTATES)));
Assert.assertTrue(
- InstanceValidationUtil.hasResourceAssigned(mock.dataAccessor,
TEST_CLUSTER, TEST_INSTANCE));
+ InstanceValidationUtil.isResourceAssigned(mock.dataAccessor,
TEST_INSTANCE));
}
@Test
@@ -156,7 +156,7 @@ public class TestInstanceValidationUtil {
.getProperty(argThat(new
PropertyKeyArgument(PropertyType.CURRENTSTATES)));
Assert.assertFalse(
- InstanceValidationUtil.hasResourceAssigned(mock.dataAccessor,
TEST_CLUSTER, TEST_INSTANCE));
+ InstanceValidationUtil.isResourceAssigned(mock.dataAccessor,
TEST_INSTANCE));
}
@Test
@@ -166,7 +166,7 @@ public class TestInstanceValidationUtil {
.getProperty(argThat(new
PropertyKeyArgument(PropertyType.LIVEINSTANCES)));
Assert.assertFalse(
- InstanceValidationUtil.hasResourceAssigned(mock.dataAccessor,
TEST_CLUSTER, TEST_INSTANCE));
+ InstanceValidationUtil.isResourceAssigned(mock.dataAccessor,
TEST_INSTANCE));
}
@Test
diff --git
a/helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
b/helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
index c70b9c016..1aaea7121 100644
---
a/helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
+++
b/helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
@@ -699,7 +699,7 @@ public class MaintenanceManagementService {
break;
case EMPTY_RESOURCE_ASSIGNMENT:
healthStatus.put(HealthCheck.EMPTY_RESOURCE_ASSIGNMENT.name(),
- InstanceValidationUtil.hasResourceAssigned(_dataAccessor,
clusterId, instanceName));
+ InstanceValidationUtil.isResourceAssigned(_dataAccessor,
instanceName));
break;
case MIN_ACTIVE_REPLICA_CHECK_FAILED:
healthStatus.put(HealthCheck.MIN_ACTIVE_REPLICA_CHECK_FAILED.name(),
diff --git
a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
index 724a8ec44..d5a64ea5f 100644
---
a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
+++
b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
@@ -56,6 +56,7 @@ import
org.apache.helix.rest.server.json.instance.StoppableCheck;
import org.apache.helix.rest.server.resources.exceptions.HelixHealthException;
import org.apache.helix.rest.server.service.ClusterService;
import org.apache.helix.rest.server.service.ClusterServiceImpl;
+import org.apache.helix.util.InstanceValidationUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -120,8 +121,7 @@ public class InstancesAccessor extends
AbstractHelixResource {
InstanceConfig instanceConfig =
accessor.getProperty(accessor.keyBuilder().instanceConfig(instanceName));
if (instanceConfig != null) {
- if (!instanceConfig.getInstanceEnabled() ||
(clusterConfig.getDisabledInstances() != null
- &&
clusterConfig.getDisabledInstances().containsKey(instanceName))) {
+ if (!InstanceValidationUtil.isInstanceEnabled(instanceConfig,
clusterConfig)) {
disabledNode.add(JsonNodeFactory.instance.textNode(instanceName));
}
