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 <joe.w...@gmail.com> 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 <trk...@gmail.com> 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 >
