streams-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mfranklin <...@git.apache.org>
Subject [GitHub] incubator-streams pull request: Overhaul of LocalStreamsBuilder
Date Mon, 19 May 2014 13:12:59 GMT
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message