Open Stack

On Mon, Sep 8, 2014 at 1:14 PM, Robert Collins <robertc at robertcollins.net>
wrote:

> 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 :)
>
>
And you can always use +1, if you feel that another core should review.

-Angus


> Thats it really - below I've some arguments to support this suggestion.
>
> -Rob
>
> # 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.
>
> # 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.
>
> ## 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.
>
> ## 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.
>
> ## 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.
>
>
> -Rob
>
>
> --
> Robert Collins <rbtcollins at hp.com>
> Distinguished Technologist
> HP Converged Cloud
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140908/72d30945/attachment.html>