Reducing Committer Load for Code Reviews

[Thomas Groh <tg...@apache.org>]

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 <ie...@gmail.com>]

+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 <kl...@google.com> 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é <jb...@nanthrax.net> 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é" <jb...@nanthrax.net> 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 < robert@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 < ehudm@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 < klk@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 < tgroh@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

Re: Reducing Committer Load for Code Reviews

[Kenneth Knowles <kl...@google.com>]

@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é <jb...@nanthrax.net>
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é" <jb...@nanthrax.net> 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 < robert@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 < ehudm@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 < klk@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 < tgroh@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
>>>>>>
>>>>>

Re: Reducing Committer Load for Code Reviews

[Jean-Baptiste Onofré <jb...@nanthrax.net>]

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é" <jb...@nanthrax.net> 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 <ro...@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 <kl...@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 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é <jb...@nanthrax.net>]

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 <ro...@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 <kl...@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 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

[Chamikara Jayalath <ch...@google.com>]

+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 <ap...@google.com> 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 <he...@google.com> wrote:
>
>> +1
>>
>> On Thu, May 31, 2018 at 8:55 AM Thomas Weise <th...@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 <ec...@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> 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 <kl...@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 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é <jb...@nanthrax.net>]

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é <jb@nanthrax.net
> <ma...@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 <herohde@google.com
>     <ma...@google.com>
>     > <mailto:herohde@google.com <ma...@google.com>>> wrote:
>     >
>     >     +1 
>     >
>     >     On Thu, May 31, 2018 at 8:55 AM Thomas Weise <thw@apache.org
>     <ma...@apache.org>
>     >     <mailto:thw@apache.org <ma...@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
>     >         <echauchot@apache.org <ma...@apache.org>
>     <mailto:echauchot@apache.org <ma...@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
>     <ehudm@google.com <ma...@google.com>
>     >>             <mailto:ehudm@google.com <ma...@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
>     >>>             <klk@google.com <ma...@google.com>
>     <mailto:klk@google.com <ma...@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
>     >>>>             <tgroh@apache.org <ma...@apache.org>
>     <mailto:tgroh@apache.org <ma...@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é
>     jbonofre@apache.org <ma...@apache.org>
>     http://blog.nanthrax.net
>     Talend - http://www.talend.com
> 
> -- 
> Got feedback? go/pabloem-feedback

-- 
--
Jean-Baptiste Onofré
jbonofre@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com

Re: Reducing Committer Load for Code Reviews

[Kenneth Knowles <kl...@google.com>]

On Thu, May 31, 2018, 15:08 Eugene Kirpichov <ki...@google.com> wrote:

>
>
> On Thu, May 31, 2018 at 2:56 PM Ismaël Mejía <ie...@gmail.com> 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 <ki...@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 <kl...@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 <ro...@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 <pa...@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é <
>> jb@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 <herohde@google.com
>> >>>>> > <ma...@google.com>> wrote:
>> >>>>> >
>> >>>>> >     +1
>> >>>>> >
>> >>>>> >     On Thu, May 31, 2018 at 8:55 AM Thomas Weise <thw@apache.org
>> >>>>> >     <ma...@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
>> >>>>> >         <echauchot@apache.org <ma...@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 <
>> ehudm@google.com
>> >>>>> >>             <ma...@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
>> >>>>> >>>             <klk@google.com <ma...@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
>> >>>>> >>>>             <tgroh@apache.org <ma...@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é
>> >>>>> jbonofre@apache.org
>> >>>>> http://blog.nanthrax.net
>> >>>>> Talend - http://www.talend.com
>> >>>>
>> >>>> --
>> >>>> Got feedback? go/pabloem-feedback
>> <https://goto.google.com/pabloem-feedback>
>>
>

Re: Reducing Committer Load for Code Reviews

[Eugene Kirpichov <ki...@google.com>]

On Thu, May 31, 2018 at 2:56 PM Ismaël Mejía <ie...@gmail.com> 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 <ki...@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 <kl...@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 <ro...@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 <pa...@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é <
> jb@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 <herohde@google.com
> >>>>> > <ma...@google.com>> wrote:
> >>>>> >
> >>>>> >     +1
> >>>>> >
> >>>>> >     On Thu, May 31, 2018 at 8:55 AM Thomas Weise <thw@apache.org
> >>>>> >     <ma...@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
> >>>>> >         <echauchot@apache.org <ma...@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 <
> ehudm@google.com
> >>>>> >>             <ma...@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
> >>>>> >>>             <klk@google.com <ma...@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
> >>>>> >>>>             <tgroh@apache.org <ma...@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é
> >>>>> jbonofre@apache.org
> >>>>> http://blog.nanthrax.net
> >>>>> Talend - http://www.talend.com
> >>>>
> >>>> --
> >>>> Got feedback? go/pabloem-feedback
> <https://goto.google.com/pabloem-feedback>
>

Re: Reducing Committer Load for Code Reviews

[Ismaël Mejía <ie...@gmail.com>]

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 <ki...@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 <kl...@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 <ro...@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 <pa...@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é <jb...@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 <herohde@google.com
>>>>> > <ma...@google.com>> wrote:
>>>>> >
>>>>> >     +1
>>>>> >
>>>>> >     On Thu, May 31, 2018 at 8:55 AM Thomas Weise <thw@apache.org
>>>>> >     <ma...@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
>>>>> >         <echauchot@apache.org <ma...@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 <ehudm@google.com
>>>>> >>             <ma...@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
>>>>> >>>             <klk@google.com <ma...@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
>>>>> >>>>             <tgroh@apache.org <ma...@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é
>>>>> jbonofre@apache.org
>>>>> http://blog.nanthrax.net
>>>>> Talend - http://www.talend.com
>>>>
>>>> --
>>>> Got feedback? go/pabloem-feedback

Re: Reducing Committer Load for Code Reviews

[Eugene Kirpichov <ki...@google.com>]

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 <kl...@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 <ro...@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 <pa...@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é <jb...@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 <herohde@google.com
>>>> > <ma...@google.com>> wrote:
>>>> >
>>>> >     +1
>>>> >
>>>> >     On Thu, May 31, 2018 at 8:55 AM Thomas Weise <thw@apache.org
>>>> >     <ma...@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
>>>> >         <echauchot@apache.org <ma...@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 <
>>>> ehudm@google.com
>>>> >>             <ma...@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
>>>> >>>             <klk@google.com <ma...@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
>>>> >>>>             <tgroh@apache.org <ma...@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é
>>>> jbonofre@apache.org
>>>> http://blog.nanthrax.net
>>>> Talend - http://www.talend.com
>>>>
>>> --
>>> Got feedback? go/pabloem-feedback
>>> <https://goto.google.com/pabloem-feedback>
>>>
>>

Re: Reducing Committer Load for Code Reviews

[Kenneth Knowles <kl...@google.com>]

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 <ro...@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 <pa...@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é <jb...@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 <herohde@google.com
>>> > <ma...@google.com>> wrote:
>>> >
>>> >     +1
>>> >
>>> >     On Thu, May 31, 2018 at 8:55 AM Thomas Weise <thw@apache.org
>>> >     <ma...@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
>>> >         <echauchot@apache.org <ma...@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 <ehudm@google.com
>>> >>             <ma...@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
>>> >>>             <klk@google.com <ma...@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
>>> >>>>             <tgroh@apache.org <ma...@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é
>>> jbonofre@apache.org
>>> http://blog.nanthrax.net
>>> Talend - http://www.talend.com
>>>
>> --
>> Got feedback? go/pabloem-feedback
>> <https://goto.google.com/pabloem-feedback>
>>
>

Re: Reducing Committer Load for Code Reviews

[Robert Bradshaw <ro...@google.com>]

+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 <pa...@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é <jb...@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 <herohde@google.com
>> > <ma...@google.com>> wrote:
>> >
>> >     +1
>> >
>> >     On Thu, May 31, 2018 at 8:55 AM Thomas Weise <thw@apache.org
>> >     <ma...@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
>> >         <echauchot@apache.org <ma...@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 <ehudm@google.com
>> >>             <ma...@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
>> >>>             <klk@google.com <ma...@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
>> >>>>             <tgroh@apache.org <ma...@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é
>> jbonofre@apache.org
>> http://blog.nanthrax.net
>> Talend - http://www.talend.com
>>
> --
> Got feedback? go/pabloem-feedback
> <https://goto.google.com/pabloem-feedback>
>

Re: Reducing Committer Load for Code Reviews

[Pablo Estrada <pa...@google.com>]

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é <jb...@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 <herohde@google.com
> > <ma...@google.com>> wrote:
> >
> >     +1
> >
> >     On Thu, May 31, 2018 at 8:55 AM Thomas Weise <thw@apache.org
> >     <ma...@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
> >         <echauchot@apache.org <ma...@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 <ehudm@google.com
> >>             <ma...@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
> >>>             <klk@google.com <ma...@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
> >>>>             <tgroh@apache.org <ma...@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é
> jbonofre@apache.org
> http://blog.nanthrax.net
> Talend - http://www.talend.com
>
-- 
Got feedback? go/pabloem-feedback

Re: Reducing Committer Load for Code Reviews

[Jean-Baptiste Onofré <jb...@nanthrax.net>]

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 <herohde@google.com
> <ma...@google.com>> wrote:
> 
>     +1 
> 
>     On Thu, May 31, 2018 at 8:55 AM Thomas Weise <thw@apache.org
>     <ma...@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
>         <echauchot@apache.org <ma...@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 <ehudm@google.com
>>             <ma...@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
>>>             <klk@google.com <ma...@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
>>>>             <tgroh@apache.org <ma...@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é
jbonofre@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com

Re: Reducing Committer Load for Code Reviews

[Andrew Pilloud <ap...@google.com>]

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 <he...@google.com> wrote:

> +1
>
> On Thu, May 31, 2018 at 8:55 AM Thomas Weise <th...@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 <ec...@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> 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 <kl...@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 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

[Henning Rohde <he...@google.com>]

+1

On Thu, May 31, 2018 at 8:55 AM Thomas Weise <th...@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 <ec...@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> 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 <kl...@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 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 <th...@apache.org>]

+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 <ec...@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> 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 <kl...@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 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 <ec...@apache.org>]

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> 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 <kl...@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 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 <ro...@frantil.com>]

+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 <kl...@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 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 <eh...@google.com>]

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

[Kenneth Knowles <kl...@google.com>]

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