Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/166#issuecomment-50172155
Yes, feel free to do the merge on another JIRA. The code looks good.
There are a lot of white space changes to Config.java, but I don't think that
is too critical.
I would like to see ShellBasedUnixGroupsMapping drop the Unix in it's name.
Looks like it will work with Windows too. Also it would be good to have some
tests for this code. I am fine with some simple unit tests that look at the
groups code in isolation. I realize it may be hard to do this cleanly for
Windows/Unix etc, especially when you don't know what user is running the
tests. But perhaps just a sanity test that you can get something back without
it blowing up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
