helix-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jiajunw...@apache.org
Subject [helix] branch master updated: Remove the unnecessary checks to allow cleaning up map/list fields in the ClusterConfig and InstanceConfig. (#1215)
Date Fri, 07 Aug 2020 20:08:03 GMT
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 <jjwang@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,


Mime
View raw message