Joe(s), as you mentioned, even if we have a non-committer review, it can’t be merged until a committer decides whether to accept whatever decision was provided. So, the burden is still on committers as to whether it’s really a +1 or not. And presumably this should only happen if there’s some evidence that the right things happened.
So, I think the big question is how much to encourage non-committers to review. And, I think there’s probably more to gain from trying to be very encouraging than not. It builds community, it can be a teaching moment, and it can offload work where there isn’t enough time or expertise. This would be really easy if the project required a double +1 to merge because there wouldn’t be any confusion about whether any one reviewer had sole authority, but I think if the developer guide and README was very encouraging and very explicit about how non-committer reviews would be treated, it’ll be easy to roll with. On Jul 7, 2017, 9:10 AM -0500, Joe Witt <[email protected]>, wrote: > We're looking at a possible set of scenarios comprised of a > 'submitter' and a 'reviewer'. There can be one or more reviewers. A > submitter or reviewer is either a committer or is not a committer. If > there is at least one reviewer that is a committer it is a committer > reviewed scenario. So with that in mind... > > A) Committer submitted, Non-Committer reviewed = This can be merged in > the event of a positive review outcome. > > B) Committer submitted, Committer reviewed = This can be merged in the > event of a positive review outcome. > > C) Non-Committer submitted, Non-Committer reviewed = This cannot be > merged until there is at least one positive committer review. > However, that review is made easier for the committer as someone who > is still building merit in the community has already done work here > and further this is a great chance to work with/grow those folks in > the community. > > D) Non-committer submitted, Committer reviewed = This can be merged in > the event of a positive review outcome. > > Scenario B and D is easy and probably there is no discussion needed. > Same for scenario C as there has to be at least one committer involved > for the code to get pushed in the first place. I suspect it is only > scenario A where we have some 'yeah but...' sort of thoughts going on. > > I am favorable to us allowing scenario A to result in a merge because > at the end of the day there is a committer involved, heavily involved > in fact, and they've already been voted by the community to be a > committer. That is a high bar as it should be. > > The risk of bad commits getting through needs to be balanced against > the risk of community growth being hampered by slow review cycles. > Let's be honest, we need the help with review bandwidth. It is really > hard to keep up and to my mind one of the best ways to annoy/deter > contributors from advancing is to make the time between contribution > to feedback take a long time. > > > On Fri, Jul 7, 2017 at 9:58 AM, Joe Skora <[email protected]> wrote: > > I agree that non-committer review make more sense if the reviewer has > > established some level of credibility and involvement in the community. > > > > Ultimately, the final committer has to be a community member and they will > > have final responsibility for the code being Apache compliant and NiFi > > ready, so even reviews by less well known members of the community should > > still be low risk. > > > > On Fri, Jul 7, 2017 at 9:24 AM, Bryan Bende <[email protected]> wrote: > > > > > I agree with encouraging reviews from everyone, but I lean towards > > > "binding" reviews coming from committers. > > > > > > If we allow any review to be binding, there could be completely > > > different levels of review that occur... > > > > > > There could be someone who isn't a committer yet, but has been > > > contributing already and will probably do a very thorough review of > > > someone's contribution, and there could be someone else who we never > > > interacted with us before and writes "+1 LGTM" on a PR and we have no > > > idea if they know what they are talking about or if they even tried > > > the contribution to see if it works. Obviously a committer can also > > > write "+1 LGTM", but I think when that comes from a committer it holds > > > more weight. > > > > > > I think we may also want to clarify if we are only talking about > > > "submitted by committer, reviewed by non-committer" or also talking > > > about "submitted by non-committer, reviewed by non-committer". > > > > > > For the first case I can see the argument that since the contribution > > > is from a committer who is already trusted, they can make the right > > > judgement call based on the review. Where as in the second case, just > > > because a community member submitted something and another community > > > member says it looks good, doesn't necessarily mean a committer should > > > come along and automatically merge it in. > > > > > > > > > On Fri, Jul 7, 2017 at 4:13 AM, Michael Hogue > > > <[email protected]> wrote: > > > > Thanks for fielding the question, Tony. > > > > > > > > Joe and James' statements both make sense. I suppose a case by case > > > > analysis could be carried out, too. For example, since I'm mostly > > > > unfamiliar with the code base but am looking to gain familiarity, I'm > > > > reviewing pretty straightforward or trivial PRs. My plan was to continue > > > > doing that until I felt comfortable reviewing something with a larger > > > > impact, such as the new TCPListenRecord processor implementation [1]. > > > > However, as Tony explained, my original question was whether that sort > > > > of > > > > review would be binding or whether I should be doing it at all. I think > > > > both of those questions were answered here in that ultimately committer > > > > sign off is needed, but reviews may be binding regardless of source. > > > > > > > > Thanks for the feedback! > > > > > > > > Mike > > > > > > > > > > > > [1] https://github.com/apache/nifi/pull/1987 > > > > On Fri, Jul 7, 2017 at 01:14 James Wing <[email protected]> wrote: > > > > > > > > > We should definitely encourage review feedback from non-committers. > > > > > Getting additional perspectives, interest, and enthusiasm from users > > > > > is > > > > > critical for any project, doubly so for an integrating application > > > > > where > > > > > committers cannot be experts in all the systems we are integrating > > > with. I > > > > > believe NiFi could use more review bandwidth. Are missing out on > > > potential > > > > > reviewers because of the current policy? > > > > > > > > > > I do not have any experience with non-committer "binding reviews" as > > > > > described in the Apache Gossip thread. How would that work? Wouldn't a > > > > > committer have to review the review and decide to commit? If we knew > > > the > > > > > reviewer well enough to accept their judgement, why not make them a > > > > > committer? > > > > > > > > > > My expectation is that many non-committer reviews are helpful and > > > > > constructive, but not necessarily 100% comprehensive. Reviewers might > > > > > comment on the JIRA ticket without working with the PR, or try the > > > proposed > > > > > change without reviewing the code, tests, etc. All great stuff, but > > > > > backstopped by committers. > > > > > > > > > > Thanks, > > > > > > > > > > James > > > > > > > > > > On Thu, Jul 6, 2017 at 7:30 PM, Joe Witt <[email protected]> wrote: > > > > > > > > > > > It is undefined at this point and I agree we should reach consensus > > > > > > and document it. > > > > > > > > > > > > I am in favor making non-committer reviews binding. > > > > > > > > > > > > Why do we do RTC: > > > > > > - To help bring along new committers/grow the community > > > > > > - To help promote quality by having peer reviews > > > > > > > > > > > > Enabling non-committer reviews to be binding still allows both of > > > > > > those to be true. > > > > > > > > > > > > On Thu, Jul 6, 2017 at 10:10 PM, Tony Kurc <[email protected]> wrote: > > > > > > > All, I was having a discussion with Mike Hogue - a recent > > > contributor - > > > > > > off > > > > > > > list, and he had some questions about the review process. And > > > largely > > > > > the > > > > > > > root of the question was, if a non-committer reviews a patch or PR > > > > > (which > > > > > > > Mike has spent some time doing), is that considered a "review"? I > > > > > didn't > > > > > > > have the answers, so I went on a hunt for documentation. I started > > > with > > > > > > the > > > > > > > Contributor Guide [1]. The guide describes reviewing, and calls > > > > > > > out > > > a > > > > > > > Reviewer role, but doesn't specifically point out that Reviewer > > > > > > > is a > > > > > > > committer, just that a committer "can actively promote > > > > > > > contributions > > > > > into > > > > > > > the repository", and goes on to imply the non-committers can > > > > > > > review. > > > > > > > > > > > > > > Given this, I was unable to answer this question: > > > > > > > If a committer "X" submits a patch or PR, it is reviewed by a > > > > > > non-committer > > > > > > > "Y", does that review satisfy the RTC requirement, and "X" may > > > merge in > > > > > > the > > > > > > > patch? > > > > > > > > > > > > > > I found a related discussion on the email list in March [2], > > > > > > > which I > > > > > > think > > > > > > > implies at least some of the community assumed the canonical > > > > > > > review > > > > > must > > > > > > be > > > > > > > by a committer. I also went back a bit to early days [3], where > > > Benson > > > > > > > implied a much less "formal" review process. > > > > > > > > > > > > > > What I'm hoping for is hopefully come to a consensus of what our > > > > > project > > > > > > > expectations are and document in the Contributor Guide. My > > > expectation > > > > > > was > > > > > > > that non-committers could review, similar to what Sean discussed > > > > > > > on > > > > > this > > > > > > > thread for Apache Gossip (incubating) [4]. However, I don't > > > > > > > believe > > > > > this > > > > > > is > > > > > > > the current consensus. > > > > > > > > > > > > > > Thoughts? > > > > > > > > > > > > > > Tony > > > > > > > > > > > > > > 1. > > > > > > > https://cwiki.apache.org/confluence/display/NIFI/Contributor > > > > > > +Guide#ContributorGuide-CodeReviewProcess > > > > > > > 2. > > > > > > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mb > > > > > > ox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q% > > > > > > 3Dmg%40mail.gmail.com%3E > > > > > > > 3. > > > > > > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mb > > > > > > ox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@ > > > > > > mail.gmail.com%3E > > > > > > > 4. > > > > > > > http://mail-archives.apache.org/mod_mbox/incubator-gossip-de > > > > > > v/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp% > > > 3Dk7wUpoVgQ% > > > > > > 40mail.gmail.com%3E > > > > > > > > > > > > > >
