Github user mfranklin commented on the pull request:
https://github.com/apache/incubator-streams/pull/16#issuecomment-43500256
Thanks for the patch. However, there are a few issues I see with it right
off the bat:
1) It includes commits from another author. Your pull requests should only
contain your code.
2) It addresses more than one issue. Please submit 1 pull request for 1
issue. It is very difficult for us to review, test and validate the changes if
they touch separate functional issues. Things like the tests are great, but we
need those added separately than a fix for the monitor threads not shutting
down.
3) The addition of isRunning and shutdown to the StreamsResultSet breaks
the pattern of a ResultSet. Granted, the class is documented poorly, so the
intent may have gotten lost, but having a Result Set manage a shared data
structure does not seem correct. If you wanted to add or change the provider
pattern to be shared queue based, then I would propose it to the list, get
consensus (or lazy consensus) and then provide an implementation. Since the
pattern was already established, the community needs to have the opportunity to
discuss before changing what is there.
---
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.
---
