You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@helix.apache.org by ji...@apache.org on 2020/08/07 20:08:03 UTC
[helix] branch master updated: Remove the unnecessary checks to
allow cleaning up map/list fields in the ClusterConfig and InstanceConfig.
(#1215)
This is an automated email from the ASF dual-hosted git repository.
jiajunwang 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 97fd6d4 Remove the unnecessary checks to allow cleaning up map/list fields in the ClusterConfig and InstanceConfig. (#1215)
97fd6d4 is described below
commit 97fd6d4bb977e02f8d3e91c3adac65a3fce1e2b4
Author: Jiajun Wang <jj...@linkedin.com>
AuthorDate: Fri Aug 7 13:07:51 2020 -0700
Remove the unnecessary checks to allow cleaning up map/list fields in the ClusterConfig and InstanceConfig. (#1215)
The null check or empty check disallow class users to clean up the corresponding fields.
This change removes these checks in the following methods,
- ClusterConfig.setDefaultCapacityMap
- ClusterConfig.setGlobalRebalancePreference
- ClusterConfig.setAbnormalStateResolverMap
- InstanceConfig.setInstanceCapacityMap
---
.../java/org/apache/helix/model/ClusterConfig.java | 85 +++++++++++--------
.../org/apache/helix/model/InstanceConfig.java | 30 ++++---
.../org/apache/helix/model/TestClusterConfig.java | 96 +++++++++++++++++-----
.../org/apache/helix/model/TestInstanceConfig.java | 29 ++++---
4 files changed, 153 insertions(+), 87 deletions(-)
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 b5b720c..f6a6481 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
@@ -706,11 +706,12 @@ public class ClusterConfig extends HelixProperty {
/**
* Set the required Instance Capacity Keys.
- * @param capacityKeys
+ * @param capacityKeys - the capacity key list.
+ * If null, the capacity keys item will be removed from the config.
*/
public void setInstanceCapacityKeys(List<String> capacityKeys) {
- if (capacityKeys == null || capacityKeys.isEmpty()) {
- throw new IllegalArgumentException("The input instance capacity key list is empty.");
+ if (capacityKeys == null) {
+ _record.getListFields().remove(ClusterConfigProperty.INSTANCE_CAPACITY_KEYS.name());
}
_record.setListField(ClusterConfigProperty.INSTANCE_CAPACITY_KEYS.name(), capacityKeys);
}
@@ -744,7 +745,8 @@ public class ClusterConfig extends HelixProperty {
* If the instance capacity is not configured in either Instance Config nor Cluster Config, the
* cluster topology is considered invalid. So the rebalancer may stop working.
* @param capacityDataMap - map of instance capacity data
- * @throws IllegalArgumentException - when any of the data value is a negative number or when the map is empty
+ * If null, the default capacity map item will be removed from the config.
+ * @throws IllegalArgumentException - when any of the data value is a negative number
*/
public void setDefaultInstanceCapacityMap(Map<String, Integer> capacityDataMap)
throws IllegalArgumentException {
@@ -769,7 +771,8 @@ public class ClusterConfig extends HelixProperty {
* If the partition weight is not configured in either Resource Config nor Cluster Config, the
* cluster topology is considered invalid. So the rebalancer may stop working.
* @param weightDataMap - map of partition weight data
- * @throws IllegalArgumentException - when any of the data value is a negative number or when the map is empty
+ * If null, the default weight map item will be removed from the config.
+ * @throws IllegalArgumentException - when any of the data value is a negative number
*/
public void setDefaultPartitionWeightMap(Map<String, Integer> weightDataMap)
throws IllegalArgumentException {
@@ -788,39 +791,43 @@ public class ClusterConfig extends HelixProperty {
private void setDefaultCapacityMap(ClusterConfigProperty capacityPropertyType,
Map<String, Integer> capacityDataMap) throws IllegalArgumentException {
if (capacityDataMap == null) {
- throw new IllegalArgumentException("Default capacity data is null");
+ _record.getMapFields().remove(capacityPropertyType.name());
+ } else {
+ Map<String, String> data = new HashMap<>();
+ capacityDataMap.entrySet().stream().forEach(entry -> {
+ if (entry.getValue() < 0) {
+ throw new IllegalArgumentException(String
+ .format("Default capacity data contains a negative value: %s = %d", entry.getKey(),
+ entry.getValue()));
+ }
+ data.put(entry.getKey(), Integer.toString(entry.getValue()));
+ });
+ _record.setMapField(capacityPropertyType.name(), data);
}
- Map<String, String> data = new HashMap<>();
- capacityDataMap.entrySet().stream().forEach(entry -> {
- if (entry.getValue() < 0) {
- throw new IllegalArgumentException(String
- .format("Default capacity data contains a negative value: %s = %d", entry.getKey(),
- entry.getValue()));
- }
- data.put(entry.getKey(), Integer.toString(entry.getValue()));
- });
- _record.setMapField(capacityPropertyType.name(), data);
}
/**
* Set the global rebalancer's assignment preference.
* @param preference A map of the GlobalRebalancePreferenceKey and the corresponding weight.
* The ratio of the configured weights will determine the rebalancer's behavior.
+ * If null, the preference item will be removed from the config.
*/
public void setGlobalRebalancePreference(Map<GlobalRebalancePreferenceKey, Integer> preference) {
- Map<String, String> preferenceMap = new HashMap<>();
-
- preference.entrySet().stream().forEach(entry -> {
- if (entry.getValue() > MAX_REBALANCE_PREFERENCE
- || entry.getValue() < MIN_REBALANCE_PREFERENCE) {
- throw new IllegalArgumentException(String
- .format("Invalid global rebalance preference configuration. Key %s, Value %d.",
- entry.getKey().name(), entry.getValue()));
- }
- preferenceMap.put(entry.getKey().name(), Integer.toString(entry.getValue()));
- });
-
- _record.setMapField(ClusterConfigProperty.REBALANCE_PREFERENCE.name(), preferenceMap);
+ if (preference == null) {
+ _record.getMapFields().remove(ClusterConfigProperty.REBALANCE_PREFERENCE.name());
+ } else {
+ Map<String, String> preferenceMap = new HashMap<>();
+ preference.entrySet().stream().forEach(entry -> {
+ if (entry.getValue() > MAX_REBALANCE_PREFERENCE
+ || entry.getValue() < MIN_REBALANCE_PREFERENCE) {
+ throw new IllegalArgumentException(String
+ .format("Invalid global rebalance preference configuration. Key %s, Value %d.",
+ entry.getKey().name(), entry.getValue()));
+ }
+ preferenceMap.put(entry.getKey().name(), Integer.toString(entry.getValue()));
+ });
+ _record.setMapField(ClusterConfigProperty.REBALANCE_PREFERENCE.name(), preferenceMap);
+ }
}
/**
@@ -859,14 +866,24 @@ public class ClusterConfig extends HelixProperty {
/**
* Set the abnormal state resolver class map.
+ * @param resolverMap - the resolver map
+ * If null, the resolver map item will be removed from the config.
*/
public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
- if (resolverMap.values().stream()
- .anyMatch(className -> className == null || className.isEmpty())) {
- throw new IllegalArgumentException(
- "Invalid Abnormal State Resolver Map definition. Class name cannot be empty.");
+ if (resolverMap == null) {
+ _record.getMapFields().remove(ClusterConfigProperty.ABNORMAL_STATES_RESOLVER_MAP.name());
+ } else {
+ if (resolverMap.entrySet().stream().anyMatch(e -> {
+ String stateModelDefName = e.getKey();
+ String className = e.getValue();
+ return stateModelDefName == null || stateModelDefName.isEmpty() || className == null
+ || className.isEmpty();
+ })) {
+ throw new IllegalArgumentException(
+ "Invalid Abnormal State Resolver Map definition. StateModel definition name and the resolver class name cannot be null or empty.");
+ }
+ _record.setMapField(ClusterConfigProperty.ABNORMAL_STATES_RESOLVER_MAP.name(), resolverMap);
}
- _record.setMapField(ClusterConfigProperty.ABNORMAL_STATES_RESOLVER_MAP.name(), resolverMap);
}
public Map<String, String> getAbnormalStateResolverMap() {
diff --git a/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java b/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
index 9397003..70cfbc6 100644
--- a/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
+++ b/helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
@@ -29,7 +29,6 @@ import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
-import com.google.common.base.Splitter;
import org.apache.helix.HelixException;
import org.apache.helix.HelixProperty;
import org.apache.helix.util.HelixUtil;
@@ -534,7 +533,8 @@ public class InstanceConfig extends HelixProperty {
/**
* Set the instance capacity information with an Integer mapping.
* @param capacityDataMap - map of instance capacity data
- * @throws IllegalArgumentException - when any of the data value is a negative number or when the map is incomplete
+ * If null, the capacity map item will be removed from the config.
+ * @throws IllegalArgumentException - when any of the data value is a negative number
*
* This information is required by the global rebalancer.
* @see <a href="Rebalance Algorithm">
@@ -547,21 +547,19 @@ public class InstanceConfig extends HelixProperty {
public void setInstanceCapacityMap(Map<String, Integer> capacityDataMap)
throws IllegalArgumentException {
if (capacityDataMap == null) {
- throw new IllegalArgumentException("Capacity Data is null");
+ _record.getMapFields().remove(InstanceConfigProperty.INSTANCE_CAPACITY_MAP.name());
+ } else {
+ Map<String, String> capacityData = new HashMap<>();
+ capacityDataMap.entrySet().stream().forEach(entry -> {
+ if (entry.getValue() < 0) {
+ throw new IllegalArgumentException(String
+ .format("Capacity Data contains a negative value: %s = %d", entry.getKey(),
+ entry.getValue()));
+ }
+ capacityData.put(entry.getKey(), Integer.toString(entry.getValue()));
+ });
+ _record.setMapField(InstanceConfigProperty.INSTANCE_CAPACITY_MAP.name(), capacityData);
}
-
- Map<String, String> capacityData = new HashMap<>();
-
- capacityDataMap.entrySet().stream().forEach(entry -> {
- if (entry.getValue() < 0) {
- throw new IllegalArgumentException(String
- .format("Capacity Data contains a negative value: %s = %d", entry.getKey(),
- entry.getValue()));
- }
- capacityData.put(entry.getKey(), Integer.toString(entry.getValue()));
- });
-
- _record.setMapField(InstanceConfigProperty.INSTANCE_CAPACITY_MAP.name(), capacityData);
}
@Override
diff --git a/helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java b/helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
index 0da32d8..cc3ecf3 100644
--- a/helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
+++ b/helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
@@ -62,12 +62,17 @@ public class TestClusterConfig {
Assert.assertEquals(keys, testConfig.getRecord()
.getListField(ClusterConfig.ClusterConfigProperty.INSTANCE_CAPACITY_KEYS.name()));
- }
- @Test(expectedExceptions = IllegalArgumentException.class)
- public void testSetCapacityKeysEmptyList() {
- ClusterConfig testConfig = new ClusterConfig("testId");
testConfig.setInstanceCapacityKeys(Collections.emptyList());
+
+ Assert.assertEquals(testConfig.getRecord()
+ .getListField(ClusterConfig.ClusterConfigProperty.INSTANCE_CAPACITY_KEYS.name()),
+ Collections.emptyList());
+
+ testConfig.setInstanceCapacityKeys(null);
+
+ Assert.assertTrue(testConfig.getRecord()
+ .getListField(ClusterConfig.ClusterConfigProperty.INSTANCE_CAPACITY_KEYS.name()) == null);
}
@Test
@@ -119,6 +124,17 @@ public class TestClusterConfig {
Assert.assertEquals(testConfig.getRecord()
.getMapField(ClusterConfig.ClusterConfigProperty.REBALANCE_PREFERENCE.name()),
mapFieldData);
+
+ testConfig.setGlobalRebalancePreference(Collections.emptyMap());
+
+ Assert.assertEquals(testConfig.getRecord()
+ .getMapField(ClusterConfig.ClusterConfigProperty.REBALANCE_PREFERENCE.name()),
+ Collections.emptyMap());
+
+ testConfig.setGlobalRebalancePreference(null);
+
+ Assert.assertTrue(testConfig.getRecord()
+ .getMapField(ClusterConfig.ClusterConfigProperty.REBALANCE_PREFERENCE.name()) == null);
}
@Test(expectedExceptions = IllegalArgumentException.class)
@@ -165,17 +181,17 @@ public class TestClusterConfig {
Assert.assertEquals(testConfig.getRecord().getMapField(ClusterConfig.ClusterConfigProperty.
DEFAULT_INSTANCE_CAPACITY_MAP.name()), capacityDataMapString);
- }
- @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "Default capacity data is null")
- public void testSetInstanceCapacityMapEmpty() {
- Map<String, Integer> capacityDataMap = new HashMap<>();
-
- ClusterConfig testConfig = new ClusterConfig("testConfig");
// The following operation can be done, this will clear the default values
- testConfig.setDefaultInstanceCapacityMap(capacityDataMap);
- // The following operation will fail
+ testConfig.setDefaultInstanceCapacityMap(Collections.emptyMap());
+
+ Assert.assertEquals(testConfig.getRecord().getMapField(ClusterConfig.ClusterConfigProperty.
+ DEFAULT_INSTANCE_CAPACITY_MAP.name()), Collections.emptyMap());
+
testConfig.setDefaultInstanceCapacityMap(null);
+
+ Assert.assertTrue(testConfig.getRecord().getMapField(ClusterConfig.ClusterConfigProperty.
+ DEFAULT_INSTANCE_CAPACITY_MAP.name()) == null);
}
@Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "Default capacity data contains a negative value: item3 = -3")
@@ -220,17 +236,17 @@ public class TestClusterConfig {
Assert.assertEquals(testConfig.getRecord().getMapField(ClusterConfig.ClusterConfigProperty.
DEFAULT_PARTITION_WEIGHT_MAP.name()), weightDataMapString);
- }
-
- @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "Default capacity data is null")
- public void testSetPartitionWeightMapEmpty() {
- Map<String, Integer> weightDataMap = new HashMap<>();
- ClusterConfig testConfig = new ClusterConfig("testConfig");
// The following operation can be done, this will clear the default values
- testConfig.setDefaultPartitionWeightMap(weightDataMap);
- // The following operation will fail
+ testConfig.setDefaultPartitionWeightMap(Collections.emptyMap());
+
+ Assert.assertEquals(testConfig.getRecord().getMapField(ClusterConfig.ClusterConfigProperty.
+ DEFAULT_PARTITION_WEIGHT_MAP.name()), Collections.emptyMap());
+
testConfig.setDefaultPartitionWeightMap(null);
+
+ Assert.assertTrue(testConfig.getRecord().getMapField(ClusterConfig.ClusterConfigProperty.
+ DEFAULT_PARTITION_WEIGHT_MAP.name()) == null);
}
@Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "Default capacity data contains a negative value: item3 = -3")
@@ -264,12 +280,48 @@ public class TestClusterConfig {
// Default value is empty
Assert.assertEquals(testConfig.getAbnormalStateResolverMap(), Collections.EMPTY_MAP);
// Test set
- Map<String, String> resolverMap = ImmutableMap.of(MasterSlaveSMD.name,
- MockAbnormalStateResolver.class.getName());
+ Map<String, String> resolverMap =
+ ImmutableMap.of(MasterSlaveSMD.name, MockAbnormalStateResolver.class.getName());
testConfig.setAbnormalStateResolverMap(resolverMap);
Assert.assertEquals(testConfig.getAbnormalStateResolverMap(), resolverMap);
// Test empty the map
testConfig.setAbnormalStateResolverMap(Collections.emptyMap());
Assert.assertEquals(testConfig.getAbnormalStateResolverMap(), Collections.EMPTY_MAP);
+
+ testConfig.setAbnormalStateResolverMap(null);
+ Assert.assertTrue(testConfig.getRecord()
+ .getMapField(ClusterConfig.ClusterConfigProperty.ABNORMAL_STATES_RESOLVER_MAP.name())
+ == null);
+ }
+
+ @Test
+ public void testSetInvalidAbnormalStatesResolverConfig() {
+ ClusterConfig testConfig = new ClusterConfig("testConfig");
+
+ Map<String, String> resolverMap = new HashMap<>();
+ resolverMap.put(null, MockAbnormalStateResolver.class.getName());
+ trySetInvalidAbnormalStatesResolverMap(testConfig, resolverMap);
+
+ resolverMap.clear();
+ resolverMap.put("", MockAbnormalStateResolver.class.getName());
+ trySetInvalidAbnormalStatesResolverMap(testConfig, resolverMap);
+
+ resolverMap.clear();
+ resolverMap.put(MasterSlaveSMD.name, null);
+ trySetInvalidAbnormalStatesResolverMap(testConfig, resolverMap);
+
+ resolverMap.clear();
+ resolverMap.put(MasterSlaveSMD.name, "");
+ trySetInvalidAbnormalStatesResolverMap(testConfig, resolverMap);
+ }
+
+ private void trySetInvalidAbnormalStatesResolverMap(ClusterConfig testConfig,
+ Map<String, String> resolverMap) {
+ try {
+ testConfig.setAbnormalStateResolverMap(resolverMap);
+ Assert.fail("Invalid resolver setup shall fail.");
+ } catch (IllegalArgumentException ex) {
+ // expected
+ }
}
}
diff --git a/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java b/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java
index b45e387..08390e4 100644
--- a/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java
+++ b/helix-core/src/test/java/org/apache/helix/model/TestInstanceConfig.java
@@ -19,15 +19,14 @@ package org.apache.helix.model;
* under the License.
*/
+import java.util.Collections;
+import java.util.Map;
+
import com.google.common.collect.ImmutableMap;
import org.apache.helix.zookeeper.datamodel.ZNRecord;
import org.testng.Assert;
import org.testng.annotations.Test;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
-
/**
* Created with IntelliJ IDEA.
* User: zzhang
@@ -91,26 +90,26 @@ public class TestInstanceConfig {
"item2", 2,
"item3", 3);
- Map<String, String> capacityDataMapString = ImmutableMap.of("item1", "1",
- "item2", "2",
- "item3", "3");
+ Map<String, String> capacityDataMapString =
+ ImmutableMap.of("item1", "1", "item2", "2", "item3", "3");
InstanceConfig testConfig = new InstanceConfig("testConfig");
testConfig.setInstanceCapacityMap(capacityDataMap);
Assert.assertEquals(testConfig.getRecord().getMapField(InstanceConfig.InstanceConfigProperty.
INSTANCE_CAPACITY_MAP.name()), capacityDataMapString);
- }
-
- @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "Capacity Data is null")
- public void testSetInstanceCapacityMapEmpty() {
- Map<String, Integer> capacityDataMap = new HashMap<>();
- InstanceConfig testConfig = new InstanceConfig("testConfig");
// This operation shall be done. This will clear the instance capacity map in the InstanceConfig
- testConfig.setInstanceCapacityMap(capacityDataMap);
- // This operation will fall.
+ testConfig.setInstanceCapacityMap(Collections.emptyMap());
+
+ Assert.assertEquals(testConfig.getRecord().getMapField(InstanceConfig.InstanceConfigProperty.
+ INSTANCE_CAPACITY_MAP.name()), Collections.emptyMap());
+
+ // This operation shall be done. This will remove the instance capacity map in the InstanceConfig
testConfig.setInstanceCapacityMap(null);
+
+ Assert.assertTrue(testConfig.getRecord().getMapField(InstanceConfig.InstanceConfigProperty.
+ INSTANCE_CAPACITY_MAP.name()) == null);
}
@Test(expectedExceptions = IllegalArgumentException.class,