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 # 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 <[email protected]> Distinguished Technologist HP Converged Cloud _______________________________________________ OpenStack-dev mailing list [email protected] http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
