[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
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...
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...
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...
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 topoConf) {
-return normalizeConf(topoConf).equals(normalizeConf((Map) JSONValue.parse(JSONValue.toJSONString(topoConf;
+@SuppressWarnings("unchecked")
+public static boolean isValidConf(Map topoConfIn) {
+ Map origTopoConf = normalizeConf(topoConfIn);
+ Map deserTopoConf = normalizeConf(
+ (Map)
JSONValue.parse(JSONValue.toJSONString(topoConfIn)));
+ return checkMapEquality(origTopoConf, deserTopoConf);
+}
+
+@VisibleForTesting
+static boolean checkMapEquality(Map orig, Map 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 pull request #2583: STORM-2649 More detailed check of config serializa...
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 topoConf) {
-return normalizeConf(topoConf).equals(normalizeConf((Map) JSONValue.parse(JSONValue.toJSONString(topoConf;
+@SuppressWarnings("unchecked")
+public static boolean isValidConf(Map topoConfIn) {
+ Map origTopoConf = normalizeConf(topoConfIn);
+ Map deserTopoConf = normalizeConf(
+ (Map)
JSONValue.parse(JSONValue.toJSONString(topoConfIn)));
+ return checkMapEquality(origTopoConf, deserTopoConf);
+}
+
+@VisibleForTesting
+static boolean checkMapEquality(Map orig, Map deser) {
+ MapDifference diff = Maps.difference(orig, deser);
+ if (diff.areEqual()) {
+ return true;
+ }
+ for (Map.Entry entryOnLeft :
diff.entriesOnlyOnLeft().entrySet()) {
+ LOG.warn("Config property not serializable. Name: {} - Value: {}",
entryOnLeft.getKey(),
+ entryOnLeft.getValue());
+ }
+ for (Map.Entry 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...
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 topoConf) {
-return normalizeConf(topoConf).equals(normalizeConf((Map) JSONValue.parse(JSONValue.toJSONString(topoConf;
+@SuppressWarnings("unchecked")
+public static boolean isValidConf(Map topoConfIn) {
+ Map origTopoConf = normalizeConf(topoConfIn);
+ Map deserTopoConf = normalizeConf(
+ (Map)
JSONValue.parse(JSONValue.toJSONString(topoConfIn)));
+ return checkMapEquality(origTopoConf, deserTopoConf);
+}
+
+@VisibleForTesting
+static boolean checkMapEquality(Map orig, Map deser) {
+ MapDifference 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...
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...
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 topoConf) {
-return normalizeConf(topoConf).equals(normalizeConf((Map) JSONValue.parse(JSONValue.toJSONString(topoConf;
+@SuppressWarnings("unchecked")
+public static boolean isValidConf(Map topoConfIn) {
+ Map origTopoConf = normalizeConf(topoConfIn);
+ Map deserTopoConf = normalizeConf(
+ (Map)
JSONValue.parse(JSONValue.toJSONString(topoConfIn)));
+ return confMapDiff(origTopoConf, deserTopoConf);
+}
+
+@VisibleForTesting
+static boolean confMapDiff(Map orig, Map deser) {
--- End diff --
Nitpick: Consider renaming to something like "checkMapEquality", since this
method checks map equality and returns a boolean indicating equality.
---
[GitHub] storm pull request #2583: STORM-2649 More detailed check of config serializa...
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 pull request #2583: STORM-2649 More detailed check of config serializa...
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 map0 = ImmutableMap.of();
+ Assert.assertTrue("case0", Utils.confMapDiff(map0, map0));
+ Map map1 = ImmutableMap.of("k0", ImmutableList.of(1L,
2L), "k1", ImmutableSet.of('s', 'f'),
+ "k2", "as");
+ Assert.assertTrue("case1", Utils.confMapDiff(map1, map1));
+ Map 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...
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 Date: 2018-02-25T18:32:45Z STORM-2649 More detailed check of config serialization ---
