Re: Reducing Committer Load for Code Reviews

[Kenneth Knowles]

On Thu, May 31, 2018, 15:08 Eugene Kirpichov  wrote:

>
>
> On Thu, May 31, 2018 at 2:56 PM Ismaël Mejía  wrote:
>
>> 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.
>>
> I think it is being proposed that a non-committer can start their review
> with a non-committer reviewer? Of course they still need to involve a
> committer to merge.
>

That's always been the case. In that case it is just helpful comments. No
policy change, since no policy involved.

Kenn



>
>>
>> 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 
>> 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  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 
>> 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 
>> 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.
>> 

Re: Reducing Committer Load for Code Reviews

[Eugene Kirpichov]

On Thu, May 31, 2018 at 2:56 PM Ismaël Mejía  wrote:

> 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.
>
I think it is being proposed that a non-committer can start their review
with a non-committer reviewer? Of course they still need to involve a
committer to merge.


>
> 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 
> 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  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 
> 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 
> 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 re

Re: Reducing Committer Load for Code Reviews

[Ismaël Mejía]

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  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  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  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  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é  
 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  > > wrote:
> >
> > +1
> >
> > On Thu, May 31, 2018 at 8:55 AM Thomas

Re: Reducing Committer Load for Code Reviews

[Eugene Kirpichov]

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  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 
> 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 
>> 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é 
>>> 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 >>> > > wrote:
 >
 > +1
 >
 > On Thu, May 31, 2018 at 8:55 AM Thomas Weise >>> > > 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 cod

Re: Reducing Committer Load for Code Reviews

[Kenneth Knowles]

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 
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  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é 
>> 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 >> > > wrote:
>>> >
>>> > +1
>>> >
>>> > On Thu, May 31, 2018 at 8:55 AM Thomas Weise >> > > 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
>>> > mailto:echauc...@apache.org>> wrote:
>>> >
>>> > Le jeudi 31 mai 2018 à 06:17 -0700, Robert Burke a écrit :
>>> >> 

Re: Reducing Committer Load for Code Reviews

[Robert Bradshaw]

+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  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é 
> 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 > > > wrote:
>> >
>> > +1
>> >
>> > On Thu, May 31, 2018 at 8:55 AM Thomas Weise > > > 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
>> > 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 compone

Re: Reducing Committer Load for Code Reviews

[Jean-Baptiste Onofré]

Agree, it sounds good to me.

That's basically what I proposed to the Euphoria DSL team ;)

Regards
JB

On 31/05/2018 20:35, Pablo Estrada 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é  > 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  
> > >> wrote:
> >
> >     +1 
> >
> >     On Thu, May 31, 2018 at 8:55 AM Thomas Weise  
> >     >> 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
> >         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
> 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
> >>>             mailto:k...@google.com>
> >> wrote:
>              ++1
> 
>      

Re: Reducing Committer Load for Code Reviews

[Pablo Estrada]

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é 
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  > > wrote:
> >
> > +1
> >
> > On Thu, May 31, 2018 at 8:55 AM Thomas Weise  > > 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
> > 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  >> > 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
> >>> 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 hard

Re: Reducing Committer Load for Code Reviews

[Jean-Baptiste Onofré]

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  > wrote:
> 
> +1 
> 
> On Thu, May 31, 2018 at 8:55 AM Thomas Weise  > 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
> 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 > > 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
>>> 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
 mailto:tg...@apache.org>> wrote:
> Hey all;
>
> I've been thinking recently about the process w

Re: Reducing Committer Load for Code Reviews

[Chamikara Jayalath]

+1 for the idea of reducing load on committers by involving contributors to
perform detailed reviews. I think this has been the case in practice at
least in some cases. I agree with Thomas Weise that proper long term
solution will be growing the committer base by helping existing regular
contributors to become committers.

+0 for Andrew's idea on allowing PRs where both author and contributor are
non-committers. I think this should be only done in (rare) cases where we
don't have a committer whose an expert in the component being modified.

Thanks,
Cham

On Thu, May 31, 2018 at 10:34 AM 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  wrote:
>
>> +1
>>
>> On Thu, May 31, 2018 at 8:55 AM Thomas Weise  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 
>>> 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  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  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  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

Re: Reducing Committer Load for Code Reviews

[Andrew Pilloud]

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  wrote:

> +1
>
> On Thu, May 31, 2018 at 8:55 AM Thomas Weise  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 
>> 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  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  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  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 t

Re: Reducing Committer Load for Code Reviews

[Henning Rohde]

+1

On Thu, May 31, 2018 at 8:55 AM Thomas Weise  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 
> 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  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  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  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
>>
>>
>

Re: Reducing Committer Load for Code Reviews

[Thomas Weise]

+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 
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  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  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  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
>
>

Re: Reducing Committer Load for Code Reviews

[Etienne Chauchot]

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  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  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  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
> 
> 

Re: Reducing Committer Load for Code Reviews

[Ismaël Mejía]

+1

This will help reviews go faster. And in the IO reviews makes extra
sense, because a common need is to ping external people who are not
committers but experts in the respective data stores. Of course this
puts more trust in the committers but makes sense.

On Thu, May 31, 2018 at 3:46 PM Kenneth Knowles  wrote:
>
> @JB: Yea, just talking about Beam practices, not the ASF rules which allow a 
> project to choose.
>
> @Robert & Udi: This is explicitly _not_ the norm. It hasn't really changed 
> since the beginning of the project. Here's the relevant section: 
> https://beam.apache.org/contribute/committer-guide/#always-get-to-lgtm-looks-good-to-me
>
> I have an idea where you are coming from, though :-). For the benefit of 
> having this background on thread, you may have in mind an analogy with 
> Google's process [1] which is exported in places like Chromium [2]. The 
> analogy may seem like this: an OWNER (~committer) can get any other Googler 
> (~contributor/anyone) to LGTM and then the owner can merge their own code.
>
> FWIW Chromium has three tiers: "Owners do not have to pick other owners for 
> reviews... a thorough review from any appropriate committer is sufficient". 
> Committer is a global status just like in ASF, and a committer _does_ have to 
> get another committer to review. So in this light, you could read Thomas' 
> proposal as saying "Beam is a small enough project that we don't need all 
> three tiers, but just the bottom two".
>
> OTOH tangentially while looking into this I see that GitHub can use OWNERS 
> files to automatically assign PRs to reviewers, with no associated 
> permissions enforcement. That seem to hit some pain points we've had about 
> finding the right reviewer and making sure incoming PRs are always tracked.
>
> Kenn
>
> [1] https://www.quora.com/What-is-Googles-internal-code-review-policy-process
> [2] 
> https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#OWNERS-files
>
>
> On Thu, May 31, 2018 at 6:36 AM Jean-Baptiste Onofré  
> wrote:
>>
>> By the way +1
>>
>> Two reviews is overkill. The review period is already pretty long, so it 
>> would better to increase it more ;)
>>
>> Regards
>> JB
>> Le 31 mai 2018, à 15:34, "Jean-Baptiste Onofré"  a écrit:
>>>
>>> That's not fully correct. A committer can directly commit, all depends of 
>>> the approach used in the project: commit and review or review and commit.
>>>
>>> In Beam we decided to do review and commit. So a committer or a PMC or a 
>>> contributor should create a PR.
>>> Other Apache projects allow to directly commit (or at least a PR merged 
>>> quickly).
>>>
>>> Just my €0.01 ;)
>>>
>>> Regards
>>> JB
>>> Le 31 mai 2018, à 15:17, Robert Burke < rob...@frantil.com> 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.

 On Wed, May 30, 2018, 2:23 PM Udi Meiri < 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> 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> 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 co

Re: Reducing Committer Load for Code Reviews

[Kenneth Knowles]

@JB: Yea, just talking about Beam practices, not the ASF rules which allow
a project to choose.

@Robert & Udi: This is explicitly _not_ the norm. It hasn't really changed
since the beginning of the project. Here's the relevant section:
https://beam.apache.org/contribute/committer-guide/#always-get-to-lgtm-looks-good-to-me

I have an idea where you are coming from, though :-). For the benefit of
having this background on thread, you may have in mind an analogy with
Google's process [1] which is exported in places like Chromium [2]. The
analogy may seem like this: an OWNER (~committer) can get any other Googler
(~contributor/anyone) to LGTM and then the owner can merge their own code.

FWIW Chromium has three tiers: "Owners do not have to pick other owners for
reviews... a thorough review from any appropriate committer is sufficient".
Committer is a global status just like in ASF, and a committer _does_ have
to get another committer to review. So in this light, you could read
Thomas' proposal as saying "Beam is a small enough project that we don't
need all three tiers, but just the bottom two".

OTOH tangentially while looking into this I see that GitHub can use OWNERS
files to automatically assign PRs to reviewers, with no associated
permissions enforcement. That seem to hit some pain points we've had about
finding the right reviewer and making sure incoming PRs are always tracked.

Kenn

[1]
https://www.quora.com/What-is-Googles-internal-code-review-policy-process
[2]
https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#OWNERS-files


On Thu, May 31, 2018 at 6:36 AM Jean-Baptiste Onofré 
wrote:

> By the way +1
>
> Two reviews is overkill. The review period is already pretty long, so it
> would better to increase it more ;)
>
> Regards
> JB
> Le 31 mai 2018, à 15:34, "Jean-Baptiste Onofré"  a écrit:
>>
>> That's not fully correct. A committer can directly commit, all depends of
>> the approach used in the project: commit and review or review and commit.
>>
>> In Beam we decided to do review and commit. So a committer or a PMC or a
>> contributor should create a PR.
>> Other Apache projects allow to directly commit (or at least a PR merged
>> quickly).
>>
>> Just my €0.01 ;)
>>
>> Regards
>> JB
>> Le 31 mai 2018, à 15:17, Robert Burke < rob...@frantil.com> 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.
>>>
>>> On Wed, May 30, 2018, 2:23 PM Udi Meiri < 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>
 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>
> 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 an

Re: Reducing Committer Load for Code Reviews

[Jean-Baptiste Onofré]

By the way +1

Two reviews is overkill. The review period is already pretty long, so it would 
better to increase it more ;)

Regards
JB

Le 31 mai 2018 à 15:34, à 15:34, "Jean-Baptiste Onofré"  a 
écrit:
>That's not fully correct. A committer can directly commit, all depends
>of the approach used in the project: commit and review or review and
>commit.
>
>In Beam we decided to do review and commit. So a committer or a PMC or
>a contributor should create a PR.
>Other Apache projects allow to directly commit (or at least a PR merged
>quickly).
>
>Just my €0.01 ;)
>
>Regards
>JB
>
>Le 31 mai 2018 à 15:17, à 15:17, 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.
>>
>>On Wed, May 30, 2018, 2:23 PM Udi Meiri  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 
>>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 
>>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
>

Re: Reducing Committer Load for Code Reviews

[Jean-Baptiste Onofré]

That's not fully correct. A committer can directly commit, all depends of the 
approach used in the project: commit and review or review and commit.

In Beam we decided to do review and commit. So a committer or a PMC or a 
contributor should create a PR.
Other Apache projects allow to directly commit (or at least a PR merged 
quickly).

Just my €0.01 ;)

Regards
JB

Le 31 mai 2018 à 15:17, à 15:17, 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.
>
>On Wed, May 30, 2018, 2:23 PM Udi Meiri  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 
>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 
>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

>>>

Re: Reducing Committer Load for Code Reviews

[Robert Burke]

+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.

On Wed, May 30, 2018, 2:23 PM Udi Meiri  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  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  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
>>>
>>

Re: Reducing Committer Load for Code Reviews

[Udi Meiri]

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  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  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
>>
>


smime.p7s
Description: S/MIME Cryptographic Signature

Re: Reducing Committer Load for Code Reviews

[Kenneth Knowles]

++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  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
>