You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by ghajos <gi...@git.apache.org> on 2018/03/02 14:48:23 UTC
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
GitHub user ghajos opened a pull request:
https://github.com/apache/storm/pull/2583
STORM-2649 More detailed check of config serialization
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ghajos/storm STORM-2649
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2583.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2583
----
commit 840c6c969f0fda0292c4fe3c07a3126ab1072d18
Author: Gergely Hajos <ro...@...>
Date: 2018-02-25T18:32:45Z
STORM-2649 More detailed check of config serialization
----
---
[GitHub] storm issue #2583: STORM-2649 More detailed check of config serialization
Posted by ghajos <gi...@git.apache.org>.
Github user ghajos commented on the issue:
https://github.com/apache/storm/pull/2583
Sorry for the late reply!
---
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2583#discussion_r172666072
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -1030,8 +1034,35 @@ private static Object normalizeConfValue(Object obj) {
return ret;
}
- public static boolean isValidConf(Map<String, Object> topoConf) {
- return normalizeConf(topoConf).equals(normalizeConf((Map<String, Object>) JSONValue.parse(JSONValue.toJSONString(topoConf))));
+ @SuppressWarnings("unchecked")
+ public static boolean isValidConf(Map<String, Object> topoConfIn) {
+ Map<String, Object> origTopoConf = normalizeConf(topoConfIn);
+ Map<String, Object> deserTopoConf = normalizeConf(
+ (Map<String, Object>) JSONValue.parse(JSONValue.toJSONString(topoConfIn)));
+ return checkMapEquality(origTopoConf, deserTopoConf);
+ }
+
+ @VisibleForTesting
+ static boolean checkMapEquality(Map<String, Object> orig, Map<String, Object> deser) {
+ MapDifference<String, Object> diff = Maps.difference(orig, deser);
--- End diff --
We need to guard against deser being null.
We are calling `JSONValue.parse` which returns null if it cannot parse an object. This here will end up throwing an NPE if deser is null. It would probably be better to switch to `JSONValue.parseWithException` and catch the exception and handle it instead.
---
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2583#discussion_r172427524
--- Diff: storm-client/test/jvm/org/apache/storm/utils/UtilsTest.java ---
@@ -18,16 +18,19 @@
package org.apache.storm.utils;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableList;
--- End diff --
Use non-shaded package from this line and below.
---
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2583#discussion_r172011690
--- Diff: storm-client/test/jvm/org/apache/storm/utils/UtilsTest.java ---
@@ -173,4 +176,23 @@ public void isZkAuthenticationConfiguredStormServerWithPropertyTest() {
}
}
}
+
+ @Test
+ public void testMapDiff() {
+ Map<String, Object> map0 = ImmutableMap.of();
+ Assert.assertTrue("case0", Utils.confMapDiff(map0, map0));
+ Map<String, Object> map1 = ImmutableMap.of("k0", ImmutableList.of(1L, 2L), "k1", ImmutableSet.of('s', 'f'),
+ "k2", "as");
+ Assert.assertTrue("case1", Utils.confMapDiff(map1, map1));
+ Map<String, Object> map2 = ImmutableMap.of("k0", ImmutableList.of(1L, 2L), "k1", ImmutableSet.of('s', 'f'),
+ "k2", "as");
+ Assert.assertTrue("case2", Utils.confMapDiff(map2, map2));
--- End diff --
Nit: This test is covered by line 186
---
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2583#discussion_r172011662
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -1030,8 +1034,34 @@ private static Object normalizeConfValue(Object obj) {
return ret;
}
- public static boolean isValidConf(Map<String, Object> topoConf) {
- return normalizeConf(topoConf).equals(normalizeConf((Map<String, Object>) JSONValue.parse(JSONValue.toJSONString(topoConf))));
+ @SuppressWarnings("unchecked")
+ public static boolean isValidConf(Map<String, Object> topoConfIn) {
+ Map<String, Object> origTopoConf = normalizeConf(topoConfIn);
+ Map<String, Object> deserTopoConf = normalizeConf(
+ (Map<String, Object>) JSONValue.parse(JSONValue.toJSONString(topoConfIn)));
+ return confMapDiff(origTopoConf, deserTopoConf);
+ }
+
+ @VisibleForTesting
+ static boolean confMapDiff(Map<String, Object> orig, Map<String, Object> deser) {
--- End diff --
Nitpick: Consider renaming to something like "checkMapEquality", since this method checks map equality and returns a boolean indicating equality.
---
[GitHub] storm issue #2583: STORM-2649 More detailed check of config serialization
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2583
@ghajos kindly reminder.
---
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
Posted by ghajos <gi...@git.apache.org>.
Github user ghajos commented on a diff in the pull request:
https://github.com/apache/storm/pull/2583#discussion_r178527233
--- Diff: storm-client/test/jvm/org/apache/storm/utils/UtilsTest.java ---
@@ -173,4 +176,49 @@ public void isZkAuthenticationConfiguredStormServerWithPropertyTest() {
}
}
}
+
+ @Test
--- End diff --
Found a positive test case in TestConfigValidate and added negative test cases. Should I collect some more test cases?
---
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2583#discussion_r172666722
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -1030,8 +1034,35 @@ private static Object normalizeConfValue(Object obj) {
return ret;
}
- public static boolean isValidConf(Map<String, Object> topoConf) {
- return normalizeConf(topoConf).equals(normalizeConf((Map<String, Object>) JSONValue.parse(JSONValue.toJSONString(topoConf))));
+ @SuppressWarnings("unchecked")
+ public static boolean isValidConf(Map<String, Object> topoConfIn) {
+ Map<String, Object> origTopoConf = normalizeConf(topoConfIn);
+ Map<String, Object> deserTopoConf = normalizeConf(
+ (Map<String, Object>) JSONValue.parse(JSONValue.toJSONString(topoConfIn)));
+ return checkMapEquality(origTopoConf, deserTopoConf);
+ }
+
+ @VisibleForTesting
+ static boolean checkMapEquality(Map<String, Object> orig, Map<String, Object> deser) {
+ MapDifference<String, Object> diff = Maps.difference(orig, deser);
+ if (diff.areEqual()) {
+ return true;
+ }
+ for (Map.Entry<String, Object> entryOnLeft : diff.entriesOnlyOnLeft().entrySet()) {
+ LOG.warn("Config property not serializable. Name: {} - Value: {}", entryOnLeft.getKey(),
+ entryOnLeft.getValue());
+ }
+ for (Map.Entry<String, Object> entryOnRight : diff.entriesOnlyOnRight().entrySet()) {
--- End diff --
I don't know of any situation where a key would be in one map but not the other map with the current JSON serialization/parsing. I am fine with leaving these checks in for completeness, but it might be good to change the warning messages to explain to the end user that something really odd happened with key "X", but because this should never happen in real life you might be on your own with debugging it.
---
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2583#discussion_r172667597
--- Diff: storm-client/test/jvm/org/apache/storm/utils/UtilsTest.java ---
@@ -173,4 +176,49 @@ public void isZkAuthenticationConfiguredStormServerWithPropertyTest() {
}
}
}
+
+ @Test
--- End diff --
Can we have some tests for isValidConf in addition to checkMapEquality?
---
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2583
---
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2583#discussion_r172011663
--- Diff: storm-client/test/jvm/org/apache/storm/utils/UtilsTest.java ---
@@ -173,4 +176,23 @@ public void isZkAuthenticationConfiguredStormServerWithPropertyTest() {
}
}
}
+
+ @Test
+ public void testMapDiff() {
--- End diff --
Nitpick: Consider splitting this into multiple small tests each checking one thing
---
[GitHub] storm issue #2583: STORM-2649 More detailed check of config serialization
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2583
+1 again, looks great
---
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2583#discussion_r172665583
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -1030,8 +1034,35 @@ private static Object normalizeConfValue(Object obj) {
return ret;
}
- public static boolean isValidConf(Map<String, Object> topoConf) {
- return normalizeConf(topoConf).equals(normalizeConf((Map<String, Object>) JSONValue.parse(JSONValue.toJSONString(topoConf))));
+ @SuppressWarnings("unchecked")
+ public static boolean isValidConf(Map<String, Object> topoConfIn) {
+ Map<String, Object> origTopoConf = normalizeConf(topoConfIn);
+ Map<String, Object> deserTopoConf = normalizeConf(
+ (Map<String, Object>) JSONValue.parse(JSONValue.toJSONString(topoConfIn)));
+ return checkMapEquality(origTopoConf, deserTopoConf);
+ }
+
+ @VisibleForTesting
+ static boolean checkMapEquality(Map<String, Object> orig, Map<String, Object> deser) {
--- End diff --
Please rename this as it is not generic Map Equality. It is very specific to `isValidConf` and it is confusing to have a generic name for it.
---
[GitHub] storm issue #2583: STORM-2649 More detailed check of config serialization
Posted by ghajos <gi...@git.apache.org>.
Github user ghajos commented on the issue:
https://github.com/apache/storm/pull/2583
@srdo @HeartSaVioR @revans2 Thank you for the review and the patience!
---