From db503a4576499f4fd89ea9566b14fdc7ade0b5a4 Mon Sep 17 00:00:00 2001 From: Jerry Date: Wed, 29 Jul 2015 21:00:49 -0700 Subject: [PATCH 1/4] STORM-966 ConfigValidation.DoubleValidator doesn't really validate whether the type of the object is a double --- storm-core/src/jvm/backtype/storm/Config.java | 2 +- .../jvm/backtype/storm/ConfigValidation.java | 20 ------------------- .../test/clj/backtype/storm/config_test.clj | 9 --------- 3 files changed, 1 insertion(+), 30 deletions(-) diff --git a/storm-core/src/jvm/backtype/storm/Config.java b/storm-core/src/jvm/backtype/storm/Config.java index 58e2a4a38c8..0dd489e8ea6 100644 --- a/storm-core/src/jvm/backtype/storm/Config.java +++ b/storm-core/src/jvm/backtype/storm/Config.java @@ -1194,7 +1194,7 @@ public class Config extends HashMap { * The percentage of tuples to sample to produce stats for a task. */ public static final String TOPOLOGY_STATS_SAMPLE_RATE="topology.stats.sample.rate"; - public static final Object TOPOLOGY_STATS_SAMPLE_RATE_SCHEMA = ConfigValidation.DoubleValidator; + public static final Object TOPOLOGY_STATS_SAMPLE_RATE_SCHEMA = Number.class; /** * The time period that builtin metrics data in bucketed into. diff --git a/storm-core/src/jvm/backtype/storm/ConfigValidation.java b/storm-core/src/jvm/backtype/storm/ConfigValidation.java index ce0f3dec333..a6e305736c5 100644 --- a/storm-core/src/jvm/backtype/storm/ConfigValidation.java +++ b/storm-core/src/jvm/backtype/storm/ConfigValidation.java @@ -253,26 +253,6 @@ public void validateField(String name, Object field) } }; - /** - * Validates a Double. - */ - public static Object DoubleValidator = new FieldValidator() { - @Override - public void validateField(String name, Object o) throws IllegalArgumentException { - if (o == null) { - // A null value is acceptable. - return; - } - - // we can provide a lenient way to convert int/long to double with losing some precision - if (o instanceof Number) { - return; - } - - throw new IllegalArgumentException("Field " + name + " must be an Double."); - } - }; - /** * Validates a power of 2. */ diff --git a/storm-core/test/clj/backtype/storm/config_test.clj b/storm-core/test/clj/backtype/storm/config_test.clj index 9750185e403..ac80b14eef6 100644 --- a/storm-core/test/clj/backtype/storm/config_test.clj +++ b/storm-core/test/clj/backtype/storm/config_test.clj @@ -99,15 +99,6 @@ (is (thrown-cause? java.lang.IllegalArgumentException (.validateField validator "test" [-100 (inc Integer/MAX_VALUE)]))))) -(deftest test-double-validator - (let [validator ConfigValidation/DoubleValidator] - (.validateField validator "test" nil) - (.validateField validator "test" 10) - ;; we can provide lenient way to convert int/long to double with losing precision - (.validateField validator "test" Integer/MAX_VALUE) - (.validateField validator "test" (inc Integer/MAX_VALUE)) - (.validateField validator "test" Double/MAX_VALUE))) - (deftest test-topology-workers-is-integer (let [validator (CONFIG-SCHEMA-MAP TOPOLOGY-WORKERS)] (.validateField validator "test" 42) From e7d890711fd72b3ff2eceddb5a090d24c6a89c98 Mon Sep 17 00:00:00 2001 From: Jerry Date: Thu, 30 Jul 2015 22:44:57 -0700 Subject: [PATCH 2/4] replacement of the double validator --- storm-core/src/jvm/backtype/storm/Config.java | 2 +- .../jvm/backtype/storm/ConfigValidation.java | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/storm-core/src/jvm/backtype/storm/Config.java b/storm-core/src/jvm/backtype/storm/Config.java index 0dd489e8ea6..4628bd4fc11 100644 --- a/storm-core/src/jvm/backtype/storm/Config.java +++ b/storm-core/src/jvm/backtype/storm/Config.java @@ -1194,7 +1194,7 @@ public class Config extends HashMap { * The percentage of tuples to sample to produce stats for a task. */ public static final String TOPOLOGY_STATS_SAMPLE_RATE="topology.stats.sample.rate"; - public static final Object TOPOLOGY_STATS_SAMPLE_RATE_SCHEMA = Number.class; + public static final Object TOPOLOGY_STATS_SAMPLE_RATE_SCHEMA =ConfigValidation.PositiveNumberValidator; /** * The time period that builtin metrics data in bucketed into. diff --git a/storm-core/src/jvm/backtype/storm/ConfigValidation.java b/storm-core/src/jvm/backtype/storm/ConfigValidation.java index a6e305736c5..7fd918b897a 100644 --- a/storm-core/src/jvm/backtype/storm/ConfigValidation.java +++ b/storm-core/src/jvm/backtype/storm/ConfigValidation.java @@ -253,6 +253,25 @@ public void validateField(String name, Object field) } }; + /** + * Validates a Positive Number + */ + public static Object PositiveNumberValidator = new FieldValidator() { + @Override + public void validateField(String name, Object o) throws IllegalArgumentException { + if (o == null) { + // A null value is acceptable. + return; + } + if(o instanceof Number) { + if(((Number)o).doubleValue() > 0.0) { + return; + } + } + throw new IllegalArgumentException("Field " + name + " must be a Number"); + } + }; + /** * Validates a power of 2. */ From 394d62ae39532dea42474578747a59a41e1e2d38 Mon Sep 17 00:00:00 2001 From: Boyang Jerry Peng Date: Mon, 3 Aug 2015 11:57:29 -0500 Subject: [PATCH 3/4] added united tests and changed exception message --- .../src/jvm/backtype/storm/ConfigValidation.java | 3 +-- storm-core/test/clj/backtype/storm/config_test.clj | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/storm-core/src/jvm/backtype/storm/ConfigValidation.java b/storm-core/src/jvm/backtype/storm/ConfigValidation.java index 7fd918b897a..a4897c82a0c 100644 --- a/storm-core/src/jvm/backtype/storm/ConfigValidation.java +++ b/storm-core/src/jvm/backtype/storm/ConfigValidation.java @@ -28,7 +28,6 @@ public class ConfigValidation { /** * Declares methods for validating configuration values. */ - public static interface FieldValidator { /** * Validates the given field. * @param name the name of the field. @@ -268,7 +267,7 @@ public void validateField(String name, Object o) throws IllegalArgumentException return; } } - throw new IllegalArgumentException("Field " + name + " must be a Number"); + throw new IllegalArgumentException("Field " + name + " must be a Positive Number"); } }; diff --git a/storm-core/test/clj/backtype/storm/config_test.clj b/storm-core/test/clj/backtype/storm/config_test.clj index ac80b14eef6..fa5575e10b2 100644 --- a/storm-core/test/clj/backtype/storm/config_test.clj +++ b/storm-core/test/clj/backtype/storm/config_test.clj @@ -99,6 +99,20 @@ (is (thrown-cause? java.lang.IllegalArgumentException (.validateField validator "test" [-100 (inc Integer/MAX_VALUE)]))))) +(deftest test-positive-number-validator + (let [validator ConfigValidation/PositiveNumberValidator] + (.validateField validator "test" nil) + (.validateField validator "test" 1.0) + (.validateField validator "test" 1) + (is (thrown-cause? java.lang.IllegalArgumentException + (.validateField validator "test" -1.0))) + (is (thrown-cause? java.lang.IllegalArgumentException + (.validateField validator "test" -1))) + (is (thrown-cause? java.lang.IllegalArgumentException + (.validateField validator "test" 0))) + (is (thrown-cause? java.lang.IllegalArgumentException + (.validateField validator "test" 0.0))))) + (deftest test-topology-workers-is-integer (let [validator (CONFIG-SCHEMA-MAP TOPOLOGY-WORKERS)] (.validateField validator "test" 42) From 2360fe1ed6b5407658b3942ba15f1feff029ff09 Mon Sep 17 00:00:00 2001 From: Boyang Jerry Peng Date: Tue, 4 Aug 2015 09:29:15 -0500 Subject: [PATCH 4/4] added line incorrectly deleted --- storm-core/src/jvm/backtype/storm/ConfigValidation.java | 1 + 1 file changed, 1 insertion(+) diff --git a/storm-core/src/jvm/backtype/storm/ConfigValidation.java b/storm-core/src/jvm/backtype/storm/ConfigValidation.java index a4897c82a0c..1153a4aa59c 100644 --- a/storm-core/src/jvm/backtype/storm/ConfigValidation.java +++ b/storm-core/src/jvm/backtype/storm/ConfigValidation.java @@ -28,6 +28,7 @@ public class ConfigValidation { /** * Declares methods for validating configuration values. */ + public static interface FieldValidator { /** * Validates the given field. * @param name the name of the field.