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.


---

Reply via email to