brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aled Sage <aled.s...@gmail.com>
Subject Re: Faster PR reviews
Date Tue, 02 May 2017 18:21:54 GMT
Hi Alex,

Interesting question. A few initial thoughts:

*YOML*
YOML (PR #363) is an exceptional case - we should not use that as an 
example when discussing this meta-question. The PR is 12,000 lines 
(including comments/notes), and was not discussed on the mailing list 
before it was submitted. I suggest we have a separate email thread 
specifically about merging that PR, as there are certainly very useful 
things we'd get from YOML.

*Small PRs*
We should strongly encourage small focused PRs on a single thing, 
wherever possible. That will make review faster, easier and lower risk. 
For such PRs, we should strive for review+merge within days (7 days 
being an upper bound in normal circumstances, hopefully).

We can add some brief guidelines to this effect at 
http://brooklyn.apache.org/developers/how-to-contribute.html

*Changing low-level Brooklyn*
PRs that change low-level things in Brooklyn (e.g. changes to 
config-inheritance etc) deserve thorough review. They are high-risk as 
the unforeseen consequences of the changes can be very subtle, and break 
downstream blueprints that rely on old ways of doing things.

*Light-weight Review*
I agree with you - where PRs look sensible, low-risk and unit tested we 
should take more risk and merge them sooner (even if there's not been 
time for a thorough review by the community).

Aled


On 02/05/2017 15:50, Duncan Johnston Watt wrote:
> Hi Alex
>
> This is probably covered already but I guess there needs to be an impact
> assessment (by submitter?) before something is waved through by default.
>
> Best
>
> Duncan
>
> On 2 May 2017 at 06:52, Alex Heneveld <alex.heneveld@cloudsoftcorp.com>
> wrote:
>
>> Hi Brooklyners-
>>
>> As many of you know, my YOML PR #363 [1] has been open for a while. This
>> sets up a foundation for giving better documentation and feedback and
>> hugely simplifying how we do our parsing.  However it's a very big PR.  I'm
>> eager to have people spend some time using it and ideally extending it --
>> but here I wanted to raise a meta-question:
>>
>>
>> *W**hat should we do with a PR when we aren't able to review things in as
>> much depth as we'd like?*
>>
>>
>> One option -- call it (A) -- is to say if we can't review things
>> thoroughly in a reasonable timeframe, we do a lighter review and if the PR
>> looks promising and safe we merge it.
>>
>> The other option -- call it (B) -- is to leave PRs open for as long as it
>> takes for us to do the complete review.
>>
>> I think most people have been approaching this with a mindset of (B), and
>> while that's great for code quality and shared code understanding, if we
>> can't deliver on that quickly, it's frankly anti-social.  The contributor
>> has to deal with merge conflicts (and the rudeness of his or her
>> contribution being ignored), and Brooklyn loses velocity.  My PR is an
>> extreme example but many have been affected by slow reviews, and I think
>> the expectation that reviews have to be so thorough is part of the
>> problem:  it even discourages reviewers, as if you're not an expert in an
>> area you probably don't feel qualified to review.
>>
>> We have good test coverage so product risk of (A) is small, and we have
>> great coders so I've no worry about us being able to solve problems that
>> (A) might introduce.  We should be encouraging reviewers to look at any
>> area, and we need to solve the problem of slow reviews.
>>
>>
>> *I propose that the**standard we apply is that we quickly either merge PRs
>> or identify what the contributor needs to resolve.
>>
>>
>> *I'm all for thorough reviews and shared understanding, but if we can't do
>> this quickly I suggest we are better to sacrifice those things rather than
>> block contributions, stifle innovation, and discourage reviews by insisting
>> on a standards that we struggle to sustain.
>>
>> As a general rule of thumb, maybe something like:
>>
>> (1) After 7 days of no activity on a PR we go with an "eyeball test";
>> unless the following statement is untrue we say:
>>
>> /I haven't done as much review as I'd like, but the code is clearly
>> helpful, not risky or obviously wrong  or breaking compatibility, it has
>> good test coverage, and we can reasonably expect the contributor or
>> committers to maintain it.  Leaving open a bit longer in case someone else
>> wants to review more but if nothing further in the next few days, let's
>> merge.
>>
>> /(If there are committers who are likely to be specifically interested,
>> call them out as CC.)
>>
>> (2) After 3 more days, if no activity, merge it.
>>
>> And we encourage _anyone_ to review anything.  If the above response is
>> the baseline, everyone in our community is qualified to do it or better and
>> we'll be grateful!
>>
>> Best
>> Alex
>>
>>
>> [1]  https://github.com/apache/brooklyn-server/pull/363
>>
>>
>


Mime
View raw message