Seems like enough consensus, and that this is a policy thing that should have an official vote.
On Thu, May 31, 2018 at 12:01 PM Robert Bradshaw <[email protected]> wrote: > +1, this is what I was going to propose. > > Code review serves two related, but distinct purposes. The first is just > getting a second set of eyes on the code to improve quality (call this the > LGTM). This can be done by anyone. The second is vetting whether this > contribution, in its current form, should be included in beam (call this > the approval), and is clearly in the purview, almost by definition, of > the committers. Often the same person can do both, but that's not required > (e.g. this is how reviews are handled internally at Google). > > I think we should trust committers to be able to give (or if a large > change, seek, perhaps on the list, as we do now) approval for their own > change. (Eventually we could delegate different approvers for different > subcomponents, rather than have every committer own everything.) > Regardless, we still want LGTMs for all changes. It can also make sense for > a non-committer to give an LGTM on another non-committers's code, and an > approver to take this into account, to whatever level at their discretion, > when approving code. Much of Go was developed this way out of necessity. > > I also want to point out that having non-committers review code helps more > than reducing load: it's a good opportunity for non-committers to get to > know the codebase (for both technical understandings and conventions), > interact with the community members, and make non-trivial contributions. > Reviewing code from a committer is especially valuable on all these points. > > - Robert > > > On Thu, May 31, 2018 at 11:35 AM Pablo Estrada <[email protected]> wrote: > >> In that case, does it make sense to say: >> >> - A code review by a committer is enough to merge. >> - Committers can have their PRs reviewed by non-committers that are >> familiar with the code >> - Non-committers may have their code reviewed by non-committers, but >> should have a committer do a lightweight review before merging. >> >> Do these seem like reasonable principles? >> -P. >> >> On Thu, May 31, 2018 at 11:25 AM Jean-Baptiste Onofré <[email protected]> >> wrote: >> >>> In that case, the contributor should be a committer pretty fast. >>> >>> I would prefer to keep at least a final validation from a committer to >>> guarantee the consistency of the project and anyway, only committer role >>> can merge a PR. >>> However, I fully agree that the most important is the Beam community. I >>> have no problem that non committer does the review and ask a committer >>> for final one and merge. >>> >>> Regards >>> JB >>> >>> On 31/05/2018 19:33, Andrew Pilloud wrote: >>> > If someone is trusted enough to review a committers code shouldn't they >>> > also be trusted enough to review another contributors code? As a >>> > non-committer I would get much quicker reviews if I could have other >>> > non-committers do the review, then get a committer who trusts us to >>> merge. >>> > >>> > Andrew >>> > >>> > On Thu, May 31, 2018 at 9:03 AM Henning Rohde <[email protected] >>> > <mailto:[email protected]>> wrote: >>> > >>> > +1 >>> > >>> > On Thu, May 31, 2018 at 8:55 AM Thomas Weise <[email protected] >>> > <mailto:[email protected]>> wrote: >>> > >>> > +1 to the goal of increasing review bandwidth >>> > >>> > In addition to the proposed reviewer requirement change, >>> perhaps >>> > there are other ways to contribute towards that goal as well? >>> > >>> > The discussion so far has focused on how more work can get done >>> > with the same pool of committers or how committers can get >>> their >>> > work done faster. But ASF is really about "community over code" >>> > and in that spirit maybe we can consider how community growth >>> > can lead to similar effects? One way I can think of is that >>> > besides code contributions existing committers and especially >>> > the PMC members can help more towards growing the committer >>> > base, by mentoring contributors and helping them with their >>> > contributions and learning the ASF way of doing things. That >>> > seems a way to scale the project in the long run. >>> > >>> > I'm not super excited about the concepts of "owner" and >>> > "maintainer" often found in (non ASF) projects like Kenn >>> > mentions. Depending on the exact interpretation, these have the >>> > potential of establishing an artificial barrier and limiting >>> > growth/sustainability in the contributor base. Such powers tend >>> > to be based on historical accomplishments vs. current >>> situation. >>> > >>> > Thanks, >>> > Thomas >>> > >>> > >>> > On Thu, May 31, 2018 at 7:35 AM, Etienne Chauchot >>> > <[email protected] <mailto:[email protected]>> wrote: >>> > >>> > Le jeudi 31 mai 2018 à 06:17 -0700, Robert Burke a écrit : >>> >> +1 I also thought this was the norm. >>> >> >>> >> My read of the committer/contributor guide was that a >>> >> committer couldn't unilaterally merge their own code >>> >> (approval/LGTM needs to come from someone familiar with >>> >> the component), rather than every review needs two >>> >> committers. I don't recall a requirement than each PR have >>> >> two committees attached, which I agree is burdensome >>> >> especially for new contributors. >>> > Yes me too, I thought exactly the same >>> >> >>> >> On Wed, May 30, 2018, 2:23 PM Udi Meiri <[email protected] >>> >> <mailto:[email protected]>> wrote: >>> >>> I thought this was the norm already? I have been the sole >>> >>> reviewer a few PRs by committers and I'm only a >>> contributor. >>> >>> >>> >>> +1 >>> >>> >>> >>> On Wed, May 30, 2018 at 2:13 PM Kenneth Knowles >>> >>> <[email protected] <mailto:[email protected]>> wrote: >>> >>>> ++1 >>> >>>> >>> >>>> This is good reasoning. If you trust someone with the >>> >>>> committer responsibilities [1] you should trust them to >>> >>>> find an appropriate reviewer. >>> >>>> >>> >>>> Also: >>> >>>> >>> >>>> - adds a new way for non-committers and committers to >>> bond >>> >>>> - makes committers seem less like gatekeepers because >>> >>>> it goes both ways >>> >>>> - might help clear PR backlog, improving our community >>> >>>> response latency >>> >>>> - encourages committers to code* >>> >>>> >>> >>>> Kenn >>> >>>> >>> >>>> [1] >>> https://beam.apache.org/contribute/become-a-committer/ >>> >>>> >>> >>>> *With today's system, if a committer and a few >>> >>>> non-committers are working together, then when the >>> >>>> committer writes code it is harder to get it merged >>> >>>> because it takes an extra committer. It is easier to >>> >>>> have non-committers write all the code and the committer >>> >>>> just does reviews. It is 1 committer vs 2 being >>> >>>> involved. This used to be fine when almost everyone was >>> >>>> a committer and all working on the core, but it is not >>> >>>> fine any more. >>> >>>> >>> >>>> On Wed, May 30, 2018 at 12:50 PM Thomas Groh >>> >>>> <[email protected] <mailto:[email protected]>> wrote: >>> >>>>> Hey all; >>> >>>>> >>> >>>>> I've been thinking recently about the process we have >>> >>>>> for committing code, and our current process. I'd like >>> >>>>> to propose that we change our current process to >>> >>>>> require at least one committer is present for each code >>> >>>>> review, but remove the need to have a second committer >>> >>>>> review the code prior to submission if the original >>> >>>>> contributor is a committer. >>> >>>>> >>> >>>>> Generally, if we trust someone with the ability to >>> >>>>> merge code that someone else has written, I think it's >>> >>>>> sensible to also trust them to choose a capable >>> >>>>> reviewer. We expect that all of the people that we have >>> >>>>> recognized as committers will maintain the project's >>> >>>>> quality bar - and that's true for both code they author >>> >>>>> and code they review. Given that, I think it's sensible >>> >>>>> to expect a committer will choose a reviewer who is >>> >>>>> versed in the component they are contributing to who >>> >>>>> can provide insight and will also hold up the quality >>> bar. >>> >>>>> >>> >>>>> Making this change will help spread the review load out >>> >>>>> among regular contributors to the project, and reduce >>> >>>>> bottlenecks caused by committers who have few other >>> >>>>> committers working on their same component. Obviously, >>> >>>>> this requires that committers act with the best >>> >>>>> interests of the project when they send out their code >>> >>>>> for reviews - but this is the behavior we demand before >>> >>>>> someone is recognized as a committer, so I don't see >>> >>>>> why that would be cause for concern. >>> >>>>> >>> >>>>> Yours, >>> >>>>> >>> >>>>> Thomas >>> > >>> > >>> >>> -- >>> -- >>> Jean-Baptiste Onofré >>> [email protected] >>> http://blog.nanthrax.net >>> Talend - http://www.talend.com >>> >> -- >> Got feedback? go/pabloem-feedback >> <https://goto.google.com/pabloem-feedback> >> >
