STORM-2421: support lists of childopts in DaemonConfig.

[hmcc]

Addresses https://issues.apache.org/jira/browse/STORM-2421 by:

• changing the annotation on DaemonConfig childopts to isStringOrStringList
• providing a utility method to read such values as a space-separated string

[harshach]

+1.

[revans2]

If we are going to a common type for *childopts it should be a List<String> not String. The reason for a list is that something with whitespace in it can be properly represented this breaks that.

[hmcc]

@revans2 You mean, replace this method with

public static String getConfigValueAsList(String name, Map<?, ?> conf) {
    // if the value is a list, return it
    // if the value is a string, split it by space and return the resulting list
}

and the caller can convert it to a space-separated string if needed?

[revans2]

@hmcc Yes, except the caller should not be converting it to a space separated string because it will break some use cases.

[erikdw]

@hmcc : I put a comment in the bug -- the problem isn't sufficiently specified IMHO.

[hmcc]

#2112 makes most of the checkstyle changes unnecessary, which is great! Rebased & squashed commits.

[revans2]

+1

Overall it looks really good, but one minor nit that you can probably ignore if you want to. In the future I would love to see configValue replaced as it cannot express anything but a very simple type, and it only outputs a single value at a time. But it works OK for now, and it is probably big enough that it would need a separate JIRA.

[revans2]

Minor nit: we tend to use Map<String, Object> for conf.

[mal]

Any hope of this getting backported into 1.0.x?

[revans2]

@hmcc looks like there is a merge conflict now. If you could resolve it I would be happy to check this in.

[HeartSaVioR]

@hmcc I'm really sorry but there's another merge conflict now. Please resolve this and mention me so that I can check this in soon.

[mal]

Not sure how serious this is considered, but I figured I'd raise it for discussion just in case:

if the value is a string, split it by space and return the resulting list

Does this not risk breaking backwards compat in cases where the current value is a string containing arguments with spaces?

// i.e.
-foo "hello world" => ["-foo", "\"hello", "world\""]
// or a more contrived example
-padding "     "   => ["-padding", "\"", "\""]

Perhaps there's an existing CLI parser better suited to handling the conversion to List<String>?

[revans2]

@mal that is not an issue right now. There are a lot of changes to the code, but most of that is creating some common helper methods and refactoring. The only configs really impacted by this change are the daemon childopts {nimbus,drpc,pacemaker,logviewer,ui}.childopts these were all really just read by storm.py by way of the ConfigValue command. Which would then parse them using

https://github.com/apache/storm/blob/master/bin/storm.py#L217-L234

the old code just output the value by calling .toString on it, but the new ConfigValue will first check if it is a list and stitch it back together into a String. So

"-foo \"hello world\"" => "-foo \"hello world\"" => ["-foo", "hello world"]
"-padding \"     \""   => "-padding \"     \""  => ["-padding", "     "]

but

["-foo", "hello world"] => "-foo hello world" => ["-foo", "hello", "world"]
["-padding", "    "] => "-padding     " => ["-padding"]

This is not ideal, I agree. Ultimately we need to add in an option to storm config to print out the config as a JSON string, or a set of JSON strings. Then have storm.py parse it properly and add in the arguments the right way to respect white space instead of what is happening right now.

[HeartSaVioR]

+1