Open Stack

Responses in-line.


-----Original Message-----
From: Robert Collins <robertc at robertcollins.net>
Reply: OpenStack Development Mailing List (not for usage questions) <openstack-dev at lists.openstack.org>>
Date: September 7, 2014 at 20:16:32
To: OpenStack Development Mailing List <openstack-dev at lists.openstack.org>>
Subject:  [openstack-dev] doubling our core review bandwidth

> I hope the subject got your attention :).
>  
> This might be a side effect of my having too many cosmic rays, but its
> been percolating for a bit.
>  
> tl;dr I think we should drop our 'needs 2x+2 to land' rule and instead
> use 'needs 1x+2'. We can ease up a large chunk of pressure on our
> review bottleneck, with the only significant negative being that core
> reviewers may see less of the code going into the system - but they
> can always read more to stay in shape if thats an issue :)
>  
> Thats it really - below I've some arguments to support this suggestion.
>  
> -Rob

I think that this is something that can be done on a project-by-project basis. However, I don’t disagree that
the mandate could be moved to “must have 1x+2” but leave it to the individual projects to specify how that is
implemented.

> # Costs of the current system
>  
> Perfectly good code that has been +2'd sits waiting for a second +2.
> This is a common complaint from folk suffering from review latency.
>  
> Reviewers spend time reviewing code that has already been reviewed,
> rather than reviewing code that hasn't been reviewed.

This is absolutely true. There are many times things linger with a single +2 and then become painful due to rebase
needs. This issue can be extremely frustrating (especially to newer contributors).

> # Benefits of the current system
>  
> I don't think we gain a lot from the second +2 today. There are lots
> of things we might get from it:
>  
> - we keep -core in sync with each other
> - better mentoring of non-core
> - we avoid collaboration between bad actors
> - we ensure -core see a large chunk of the changes going through the system
> - we catch more issues on the code going through by having more eyeballs
>  
> I don't think any of these are necessarily false, but equally I don't
> they are necessarily true.
>
>
> ## keeping core in sync
>  
> For a team of (say) 8 cores, if 2 see each others comments on a
> review, a minimum of 7 reviews are needed for a reviewer R's thoughts
> on something to be disseminated across the team via osmosis. Since
> such thoughts probably don't turn up on every review, the reality is
> that it may take many more reviews than that: it is a thing, but its
> not very effective vs direct discussion.

I wouldn’t discount how much benefit is added by forcing the cores to see more of the code going into the repo. I personally feel like (as a core on a project) I would be lacking a lot of insight as to the code base without the extra reviews. It might take me longer to get up to speed when reviewing or implementing something new simply because I have a less likely chance to have seen the recently merged code.

Losing this isn’t the end of the world by any means.

> ## mentoring of non-core
>  
> This really is the same as the keeping core in sync debate, except
> we're assuming that the person learning has nothing in common to start
> with.

This isn’t really a benefit to multiple core reviewers looking over a patch set from my experience. Most of the mentoring I see has been either in IRC or just because reviews (non-core even) occur. I agree with your assessment.

> ## avoiding collaboration between bad actors
>  
> The two core requirement means that it takes three people (proposer +
> 2 core) to collaborate on landing something inappropriate (whether its
> half baked, a misfeature, whatever). Thats only 50% harder than 2
> people (proposer + 1 core) and its still not really a high bar to
> meet. Further, we can revert things.

Solid assessment. I tend to agree with this point. If you are going to have bad actors try and get code in you will have bad actors trying to get code in. The real question is: how many (if any) extra reverts will be needed in the case of bad actors? My guess is 1 per bad actor (which that actor is likely no longer going to be core), if there are even any bad actors out there.

> ## Seeing a high % of changes
>  
> Consider nova - http://russellbryant.net/openstack-stats/nova-reviewers-90.txt  
> Core team size: 21 (avg 3.8 reviews/day) [79.8/day for the whole team]
> Changes merged in the last 90 days: 1139 (12.7/day)
>  
> Each reviewer can only be seeing 30% (3.8/12.7) of the changes to nova
> on average (to keep up with 12/day landing). So they're seeing a lot,
> but there's more that they aren't seeing already. Dropping 30% to 15%
> might be significant. OTOH seeing 30% is probably not enough to keep
> up with everything on its own anyway - reviewers are going to be
> hitting new code regularly.
>
> ## Catching more issues through more eyeballs
>  
> I'm absolutely sure we do catch more issues through more eyeballs -
> but what eyeballs look at any given review is pretty arbitrary. We
> have a 30% chance of any given core seeing a given review (with the
> minimum of 2 +2s). I don't see us making a substantial difference to
> the quality of the code that lands via the second +2 review. I observe
> that our big themes on quality are around systematic changes in design
> and architecture, not so much the detail of each change being made.

This is the big one for me. I can count a few times we’ve seen (at least in Keystone) a large benefit from the extra eyes that may have otherwise been missed. The issues could have been significant, possibly not caught until much later on making fixing things require getting over significantly more hurdles. A few might have even been subtle/hard to see security related bugs. This is main reason I hesitate giving strong support towards reducing the number of +2 votes to merge a patch. I am in no way trying to imply non-core reviewers are inadequate/unfamiliar with the code base, simply that with a single +2 vote a patch may get through merge much faster and miss some of these critical in-depth reviews (regardless of who the reviewers are).

>

Overall I think it is worth considering this change, however, we might get the same (rough) benefit from using the concept of the partial-core (can +2 CR but cannot workflow +1) for the larger teams. Smaller projects (and stackforge projects) are probably not hitting the issues that drive us towards considering this change.

—Morgan