brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Richard Downer <rich...@apache.org>
Subject Re: Faster PR reviews
Date Wed, 03 May 2017 22:52:25 GMT
All,

A lot of very good points going on here - thank you to everyone who has
contributed to this conversation. It's clear that we are doing a disservice
to our contributors and things need to change. We need to be merging (or,
rarely, rejecting) PRs consistently and promptly. Obviously, difficulty
comes in how we do this while keeping quality up.

After reading the debate so far, I think Alex has come up with a simple and
sensible suggestion:

On 03/05/2017 02:13, Alex Heneveld wrote:

> I'm suggesting we make more of an effort, and we fall back to an "eyeball
> test" a certain period of time (7 days max, less if it's simple?), triage
> the review to look at:
>
> * clearly helpful & not obviously wrong
> * low-risk / doesn't break compatibility
> * good test coverage (and passing)
> * likely to be maintained
>
> If these can't be confirmed, the reviewer should say what they have doubts
> about, maybe suggest what the contributor could do to help, or appeal to
> other committers more familiar with an area. In any case get a discussion
> going.
>
> If these do seem confirmed, I still suggest we don't merge immediately in
> the absence of a thorough review, but ping specific committers likely to be
> interested.  If no thorough review after a few more days, _then_ merge.
>
> I'm not suggesting any heavyweight process, but just enough to put healthy
> forces on us as reviewers.
>

And Aled makes some sensible observations around this subject:

On 03/05/2017 09:41, Aled Sage wrote:

> 1. Some PRs don't get enough attention from reviewers.
>
> 2. Not as much time as we'd like is spent by the community reviewing PRs.
>
> 3. The bar for being merged is too high (for at least some PRs).
>
> 4. Some PRs are very hard to review.
>
> There are clearly examples of (1) where it's a no-brainer we should have
> given it more attention, commented and merged. Your proposal for the
> "eye-ball test" will help for some.
>
> However, there are also examples of (1) that are caused by (4) - commonly
> due to the complexity of the code being changed (e.g. config-inheritance),
> sometimes made worse by it changing many things (so it's more daunting to
> review).
>
> Given (2) and (3), it suggests we should spread that time across more PRs
> (i.e. some PRs that are getting a very thorough review could get less, and
> folk try to use that "saved" time on some other PRs). I'm not convinced
> that would actually happen in practice though!
>
>
> *Solutions*
>
> 1. Add the "eye-ball test" to our reviewer guidelines (as described by
> Alex) - and adjust our perception of "low-risk" over time, as we see how it
> works.
>
> 2. Guidelines for PRs - what will make the reviewers' job considerably
> easier, so we can get things merged faster? For example, small and focused
> PRs, with good test coverage, linking to a jira issue where appropriate.
>
> 3. Invest more time simplifying Brooklyn (see below).
>
>
> *Complexity of Brooklyn*
> I've heard from quite a few people that certain areas of Brooklyn are far
> too hard to understand. Some people avoid reviewing PRs that touch it,
> because it's so hard to understand the implications - they focus their time
> on PRs that they feel more confident to review.
>
> This is a symptom of an overly complex project. It would be great to find
> more time to simplify things - e.g. to delete things from Brooklyn, to make
> things more consistent, to refactor or even rewrite some sections, and to
> add more javadoc.
>
>
> *Accepted Limitations to Timely Review*
> PRs that make far reaching changes to low-level details of Brooklyn will
> always require a thorough review. Clearly we should try to find the time
> for that promptly, but should always view them as high-risk.


Thomas also makes a good observation which is that `master` would have to
be considered unstable. This is true, although the committers are still
acting as gatekeepers, so it won't be a wild west out there - but it does
mean that taking the bleeding-edge code would be somewhat higher risk. This
does complicate things when we come near to release time. Some projects use
Git branching models such as [1]; other projects only accept "new feature"
PRs at certain points in the release cycle and have strict stabilisation
phases near the end of a cycle. This is something that we will need to
discuss.

Alex and Aled have both made some suggestions about how to mitigate that
risk, and I'd add that we should also push for bug fix PRs to boost test
coverage to reduce the chance of the problem happening again.

I'd also explicitly make the point that it's generally accepted in open
source communities that a contributor is expected to make some effort to
make their contribution easy to review and accept, and can expect pushback
if their contribution is poorly structured or neglects tests, for example
(Alex alludes to this as "maybe suggest what the contributor could do to
help" and Aled states it explicitly as "guidelines for PRs"). Setting up a
list of rules could be seen as strict/inflexible and possibly daunting for
a new committer, but there is an advantage in setting down some best
practices (an expansion of what we already have at [2]) and being flexible
in their interpretation. Where we are currently failing here is that when
faced with a difficult contribution, we don't push back to the contributor
and make it clear what we would like to see - instead the contribution is
just ignored.

To add some of my own opinions: this is just software, it's supposed to be
malleable and changing, and we have SCM and tests as a safety net. Let's
take some more risks. Along the way we'll inevitably make mistakes and
things will break, sometimes at the worst possible time - accept that this
is normal, and learn from the experience. We're just making a release now
so we've got a couple of months to experiment, during which time we can
tweak our interpretations of risk etc. and converge on a release-quality
build.

While some of the surrounding details need to be worked out (such as how do
we do a release if master is unstable):

*do we feel like we are converging on an acceptable position based around
Alex's eyeball review concept?*

Thanks
Richard.


[1]http://nvie.com/posts/a-successful-git-branching-model/
[2]
https://brooklyn.apache.org/developers/how-to-contribute.html#pull-request-at-github


On 3 May 2017 at 17:38, Sam Corbett <sam.corbett@cloudsoftcorp.com> wrote:

> I used GitHub to check the number of pull requests we've merged to a few
> of our projects over the last year in two month-chunks [1]. Each list
> begins today and works backwards:
>
> brooklyn-server: 86, 79, 85, 68, 95, 78.
> brooklyn-client: 7, 4, 3, 8, 2, 4.
> brooklyn-library: 11, 12, 9, 7, 17, 8.
> total: 104, 95, 97, 83, 114, 90.
>
> It's my opinion that these numbers show a consistency in the rate at which
> we merge pull requests. Since a few committers have recently joined I
> expect the rate to increase over the next few months. I would like to know
> other figures like the average length of time between opening a pull
> request and a committer merging it and the average length of time before
> the first review comment. I'm sure there are many other interesting metrics
> that could inform this discussion.
>
> Maybe there's just one or two cases that we have managed badly.
> Identifying those pull requests and understanding the cause of the delay to
> each one would be valuable.
>
> Regarding the complexity of Brooklyn and speaking as someone that has used
> it in anger (sometimes literally) I would posit that many of us have been
> caught out several times by the corner cases and unexpected interactions
> between components that complexity so often implies. This leads to cautious
> reviews and slow turnaround times. Quick-scan reviews solve the speed at
> which we get code into the product. We need to think harder about how we
> reduce its overall complexity.
>
> Sam
>
> 1. I used GitHub's interface to search for "is:pr
> merged:2017-03-04..2017-05-03", etc.
>
>
>
> On 03/05/2017 09:41, Aled Sage wrote:
>
>> Hi Alex,
>>
>> I agree with you that we have a problem with the reviewing of some of our
>> PRs - it's a bad situation for all concerned when these PRs stay open for
>> as long as they do.
>>
>> I agree with your "eye-ball test" for a certain class of PR. I think
>> where we probably disagree is where the line is for "low-risk" PRs. There
>> are some examples where they "eye-ball test" would certainly have helped.
>>
>> I deliberately expanded the scope of this discussion, because there's not
>> one single solution - I'm not "turning this discussion in to guidelines
>> about PR". I'm adding to the discussion.
>>
>>
>> *Contributing Factors*
>> Breaking this down into the contributing factors, I believe those are:
>>
>> 1. Some PRs don't get enough attention from reviewers.
>>
>> 2. Not as much time as we'd like is spent by the community reviewing PRs.
>>
>> 3. The bar for being merged is too high (for at least some PRs).
>>
>> 4. Some PRs are very hard to review.
>>
>> There are clearly examples of (1) where it's a no-brainer we should have
>> given it more attention, commented and merged. Your proposal for the
>> "eye-ball test" will help for some.
>>
>> However, there are also examples of (1) that are caused by (4) - commonly
>> due to the complexity of the code being changed (e.g. config-inheritance),
>> sometimes made worse by it changing many things (so it's more daunting to
>> review).
>>
>> Given (2) and (3), it suggests we should spread that time across more PRs
>> (i.e. some PRs that are getting a very thorough review could get less, and
>> folk try to use that "saved" time on some other PRs). I'm not convinced
>> that would actually happen in practice though!
>>
>>
>> *Solutions*
>>
>> 1. Add the "eye-ball test" to our reviewer guidelines (as described by
>> Alex) - and adjust our perception of "low-risk" over time, as we see how it
>> works.
>>
>> 2. Guidelines for PRs - what will make the reviewers' job considerably
>> easier, so we can get things merged faster? For example, small and focused
>> PRs, with good test coverage, linking to a jira issue where appropriate.
>>
>> 3. Invest more time simplifying Brooklyn (see below).
>>
>>
>> *Complexity of Brooklyn*
>> I've heard from quite a few people that certain areas of Brooklyn are far
>> too hard to understand. Some people avoid reviewing PRs that touch it,
>> because it's so hard to understand the implications - they focus their time
>> on PRs that they feel more confident to review.
>>
>> This is a symptom of an overly complex project. It would be great to find
>> more time to simplify things - e.g. to delete things from Brooklyn, to make
>> things more consistent, to refactor or even rewrite some sections, and to
>> add more javadoc.
>>
>>
>> *Accepted Limitations to Timely Review*
>> PRs that make far reaching changes to low-level details of Brooklyn will
>> always require a thorough review. Clearly we should try to find the time
>> for that promptly, but should always view them as high-risk.
>>
>>
>> *YOML*
>> If you insist on generalising YOML here, rather than a separate email
>> thread specifically about it, then: we should have commented very quickly
>> and discussed it promptly on the mailing list - at the level of whether we
>> want it (ignoring much of its technical details). If it was pretty much
>> anyone but Alex, then we should have commented saying:
>>
>>    "Very interesting, but this is a huge amount of code to add and
>>    maintain in Brooklyn. Can you instead create a new github project
>>    for this library, so that it can be worked on and maintained
>>    separately? We'd then be interested to see how it can be used within
>>    Brooklyn. Can you close this PR and let us know when/where you
>>    create that library."
>>
>> Like I said, that's for pretty much anyone but Alex. The difference is
>> that Alex wrote the first version of our yaml/camp parsing and knows it
>> better than anyone else. That original code definitely deserves a re-write:
>> it's become increasingly complicated as the supported yaml has evolved.
>> Alex has investigated different approaches and has come up with a way that
>> could greatly improve that code, and be used in other places as well. Doing
>> that in Brooklyn is simpler for him, because it can evolve in tandem to
>> satisfy requirements of Brooklyn.
>>
>> I therefore suggest we discuss YOML separately, rather than generalising.
>>
>> Aled
>>
>>
>> On 03/05/2017 02:13, Alex Heneveld wrote:
>>
>>>
>>> Aled,
>>>
>>> > *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).
>>>
>>> I'm saying something a little different:  we should _try_ for a thorough
>>> review of *all* PRs.  Which I think is uncontroversial.
>>>
>>> > What should we do with a PR when we aren't able to review things in as
>>> much depth as we'd like?
>>>
>>> This is the question I'm asking, to ensure we handle PR's in a good time
>>> frame.  To summarise, I'm suggesting we make more of an effort, and we fall
>>> back to an "eyeball test" a certain period of time (7 days max, less if
>>> it's simple?), triage the review to look at:
>>>
>>> * clearly helpful & not obviously wrong
>>> * low-risk / doesn't break compatibility
>>> * good test coverage (and passing)
>>> * likely to be maintained
>>>
>>> If these can't be confirmed, the reviewer should say what they have
>>> doubts about, maybe suggest what the contributor could do to help, or
>>> appeal to other committers more familiar with an area. In any case get a
>>> discussion going.
>>>
>>> If these do seem confirmed, I still suggest we don't merge immediately
>>> in the absence of a thorough review, but ping specific committers likely to
>>> be interested.  If no thorough review after a few more days, _then_ merge.
>>>
>>> I'm not suggesting any heavyweight process, but just enough to put
>>> healthy forces on us as reviewers.
>>>
>>> This is not a theoretical question, nor is it restricted to the YOML
>>> PR.  We're pretty good with most of our PRs and reviews but there are
>>> plenty of examples where we've dropped the ball.  Look at [1] which is tiny
>>> and tests-only and took nine days to get a review.  Or [2] which yes
>>> combines a few related-but-different things but is by no means a hard thing
>>> to review.  It would take far more time to split that up into 3 branches,
>>> test those locally, then babysit each of those PR's than it would take for
>>> a reviewer to just get on with a review.  It's been sitting there for 2
>>> months and doesn't even have a comment.
>>>
>>> This is not a good state of affairs.  Turning this discussion in to
>>> guidelines about PR's misses the point.  If there's any change to our
>>> docs/process made as a result of this discussion I'd like to see the
>>> eyeball test added to a review process discussion.
>>>
>>> Finally re YOML, there is an ML thread started when the issue was
>>> raised.  There was chatter beforehand but it wasn't an easy thing to
>>> discuss until there was prototype code.  The point is for 7 months there
>>> have been no comments in any of these places, even after I've run a public
>>> session explaining it and private sessions and the PR itself says how it
>>> can be tested and how it is insulated from the rest of the code (Thomas I
>>> think you missed that point).  As there is an ML thread and an open issue,
>>> either of which would be a fine place to comment, but no one is -- the
>>> suggestion of a new separate ML thread to solve the problem is bizarre.  I
>>> say this is _exactly_ the situation when we need guidelines for how we
>>> handle PR's that are not being reviewed in a timely way.
>>>
>>> Best
>>> Alex
>>>
>>>
>>> [1] https://github.com/apache/brooklyn-server/pull/600
>>> [2] https://github.com/apache/brooklyn-server/pull/575
>>>
>>>
>>>
>>> On 02/05/2017 19:21, Aled Sage wrote:
>>>
>>>> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message