If I understood correctly what is proposed is:

- Committers to be able to have their PRs reviewed by non-committers
and be able to self-merge.
- For non-committers nothing changes.

This enables a committer (wearing contributor head) to merge their own
changes without committer approval, so we should ensure that no
shortcuts are taken just to get things done quickly.

I think as Thomas Weise mentioned that mentoring and encouraging more
contributors to become committers is a better long term solution to
this issue.
On Thu, May 31, 2018 at 11:24 PM Eugene Kirpichov <kirpic...@google.com> wrote:
>
> Agreed with all said above - as I understand it, we have consensus on the 
> following:
>
> Whether you're a committer or not:
> - Find somebody who's familiar with the code and ask them to review. Use your 
> best judgment in whose review would give you good confidence that your code 
> is actually good.
> (it's a well-known problem that for new contributors it's often difficult to 
> choose a proper reviewer - we should discuss that separately)
>
> If you're a committer:
> - Once the reviewers are happy, you can merge the change yourself.
>
> If you're not a committer:
> - Once the reviewers are happy: if one of them is a commiter, you're done; 
> otherwise, involve a committer. They may give comments, including possibly 
> very substantial comments.
> - To minimize the chance of the latter: if your change is potentially 
> controversial, involve a committer early on, or involve the dev@ mailing 
> list, write a design doc etc.
>
> On Thu, May 31, 2018 at 2:16 PM Kenneth Knowles <k...@google.com> wrote:
>>
>> 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 <rober...@google.com> 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 <pabl...@google.com> 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é <j...@nanthrax.net> 
>>>> 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 <hero...@google.com
>>>>> > <mailto:hero...@google.com>> wrote:
>>>>> >
>>>>> >     +1
>>>>> >
>>>>> >     On Thu, May 31, 2018 at 8:55 AM Thomas Weise <t...@apache.org
>>>>> >     <mailto:t...@apache.org>> 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
>>>>> >         <echauc...@apache.org <mailto:echauc...@apache.org>> 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 <eh...@google.com
>>>>> >>             <mailto:eh...@google.com>> 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
>>>>> >>>             <k...@google.com <mailto:k...@google.com>> 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
>>>>> >>>>             <tg...@apache.org <mailto:tg...@apache.org>> 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é
>>>>> jbono...@apache.org
>>>>> http://blog.nanthrax.net
>>>>> Talend - http://www.talend.com
>>>>
>>>> --
>>>> Got feedback? go/pabloem-feedback

Reply via email to