On Thu, May 31, 2018, 15:08 Eugene Kirpichov <kirpic...@google.com> wrote:
> > > 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. > 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 <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> >> >
