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