Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/166#issuecomment-49099439
For the most part the code looks really good. I am concerned about not
having any caching in the groups implementation, even if it expires after just
a couple of seconds. I also don't think we need to expose the caching APIs
explicitly. They can probably just be an implementation detail. If you want
to put in the caching in a follow on JIRA that is fine too. Getting the API in
place is probably the most critical.
I am fine with you borrowing some code from Hadoop to make this simpler.
No need to reinvent the wheel. But there are already some Shell utilities in
storm that do similar things.
https://github.com/apache/incubator-storm/blob/master/storm-core/src/jvm/backtype/storm/utils/ShellProcess.java
It would be good to file a follow on JIRA to look at how we can combine the
two together so we don't have two classes that do almost the same thing.
---
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.
---
