+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 <k...@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é <j...@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é" <j...@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 < rob...@frantil.com> a écrit: >>>> >>>> +1 I also thought this was the norm. >>>> >>>> My read of the committer/contributor guide was that a committer couldn't >>>> unilaterally merge their own code (approval/LGTM needs to come from >>>> someone familiar with the component), rather than every review needs two >>>> committers. I don't recall a requirement than each PR have two committees >>>> attached, which I agree is burdensome especially for new contributors. >>>> >>>> On Wed, May 30, 2018, 2:23 PM Udi Meiri < eh...@google.com> wrote: >>>>> >>>>> I thought this was the norm already? I have been the sole reviewer a few >>>>> PRs by committers and I'm only a contributor. >>>>> >>>>> +1 >>>>> >>>>> On Wed, May 30, 2018 at 2:13 PM Kenneth Knowles < k...@google.com> wrote: >>>>>> >>>>>> ++1 >>>>>> >>>>>> This is good reasoning. If you trust someone with the committer >>>>>> responsibilities [1] you should trust them to find an appropriate >>>>>> reviewer. >>>>>> >>>>>> Also: >>>>>> >>>>>> - adds a new way for non-committers and committers to bond >>>>>> - makes committers seem less like gatekeepers because it goes both ways >>>>>> - might help clear PR backlog, improving our community response latency >>>>>> - encourages committers to code* >>>>>> >>>>>> Kenn >>>>>> >>>>>> [1] https://beam.apache.org/contribute/become-a-committer/ >>>>>> >>>>>> *With today's system, if a committer and a few non-committers are >>>>>> working together, then when the committer writes code it is harder to >>>>>> get it merged because it takes an extra committer. It is easier to have >>>>>> non-committers write all the code and the committer just does reviews. >>>>>> It is 1 committer vs 2 being involved. This used to be fine when almost >>>>>> everyone was a committer and all working on the core, but it is not fine >>>>>> any more. >>>>>> >>>>>> On Wed, May 30, 2018 at 12:50 PM Thomas Groh < tg...@apache.org> wrote: >>>>>>> >>>>>>> Hey all; >>>>>>> >>>>>>> I've been thinking recently about the process we have for committing >>>>>>> code, and our current process. I'd like to propose that we change our >>>>>>> current process to require at least one committer is present for each >>>>>>> code review, but remove the need to have a second committer review the >>>>>>> code prior to submission if the original contributor is a committer. >>>>>>> >>>>>>> Generally, if we trust someone with the ability to merge code that >>>>>>> someone else has written, I think it's sensible to also trust them to >>>>>>> choose a capable reviewer. We expect that all of the people that we >>>>>>> have recognized as committers will maintain the project's quality bar - >>>>>>> and that's true for both code they author 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
