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