Open Stack

On 11/14/2013 09:03 AM, Daniel P. Berrange wrote:
> On Thu, Nov 14, 2013 at 08:29:36PM +1300, Robert Collins wrote:
>> At the summit there were lots of discussions about reviews, and I made
>> the mistake of sending a mail to Russell proposing a few new stats we
>> could gather.
>>
>> I say mistake, because he did so and then some... we new have extra
>> info - consider:
>>
>> http://russellbryant.net/openstack-stats/nova-reviewers-30.txt
>>
>> There are two new things:
>> In each row a new column 'received' - this counts the number of
>> incoming reviews to each reviewer. It's use should be obvious - but
>> remember that folk who contribute the occasional patch probably don't
>> have the context to be doing reviews... those who are contributing
>> many patches and getting many incoming reviews however...
>> This gives us the philanthropists (or perhaps team supporters...)
>>
>> |        klmitch **       |     137    0  19   0 118   9    86.1% |
>> 14 ( 11.9%)  |      0 (∞)    |
>>
>> (the bit at the end is a unicode infinity... we'll need to work on that).
>>
>> And so on :)
>>
>> Down the bottom of the page:
>>
>> Total reviews: 2980 (99.3/day)
>> Total reviewers: 241
>> Total reviews by core team: 1261 (42.0/day)
>> Core team size: 17
>> New patch sets in the last 30 days: 1989 (66.3/day)
>>
>> and for 90 days:
>>
>> Total reviews: 10705 (118.9/day)
>> Total reviewers: 406
>> Total reviews by core team: 5289 (58.8/day)
>> Core team size: 17
>> New patch sets in the last 90 days: 7515 (83.5/day)
>>
>> This is the really interesting bit. Remembering that every patch needs
>> - at minimum - 2 +2's, the *minimum* viable core team review rate to
>> keep up is patch sets per day * 2:
>> 30 days: 132 core reviews/day
>> 90 days: 167 core reviews/day
>>
>> But we're getting:
>> 30 days: 42/day or 90/day short
>> 90 days: 59/day or 108/day short
>>
>> One confounding factor here is that this counts (AIUI) pushed changes,
>> not change ids - so we don't need two +2's for every push, we need two
>> +2's for every changeid - we should add that as a separate metric I
>> think, as the needed +2 count will be a bit lower.
> 
> NB, this analysis seems to assume that we actally /want/ to eventually
> approve ever submitted patch. There's going to be some portion that are
> abandoned or rejected. It is probably a reasonably small portion of the
> total so won't change the order of magnitude, but I'd be interested in
> stats on how many patches we reject in one way or another.

IIRC about 2/3 of patches eventually make their way in. Realize
eventually for cells was 12 months and 80 iterations. Jeblair had real
numbers on this for a release somewhere.

>> Anyhow, to me - this is circling in nicely on having excellent
>> information (vs data) on the review status, and from there we can
>> start to say 'right, to keep up, Nova needs N core reviewers
>> consistently doing Y reviews per day. If Y is something sensible like
>> 3 or 4, we can work backwards. Using the current figures (which since
>> we don't have changeId as a separate count are a little confounded)
>> that would give us:
>>
>> time period      reviews/core/day     core-team-size
>> 30 days           3                            44
>> 30 days           4                            33
>> 30 days           8                            17
>> 90 days           3                            56
>> 90 days           4                            42
>> 90 days           10                          17
>>
>> Also note that these are calendar days, so no weekends or leave for -core!
>>
>> What else....
>>
>> in the last 30 days core have done 42% of reviews, in the last 90 days
>> 49%. So thats getting better.
>>
>> I know Russell has had concerns about core cohesion in the past, but I
>> don't think doing 8 detailed reviews every day including weekends is
>> individually sustainable. IMO we badly need more core reviewers....
>> and that means:
>>
>>  - 20 or so volunteers
>>  - who step up and do - pick a number - say 3 - reviews a day, every
>> work day, like clockwork.
>>  - and follow up on their reviewed patches to learn what other
>> reviewers say, and why
>>  - until the nova-core team & Russell are happy that they can
>> contribute effectively as -core.
>>
>> Why such a big number of volunteers? Because we need a big number of
>> people to spread load, because Nova has a high incoming patch rate.
> 
> One concern I have is that this exercise could turn out to be a bit like
> building new roads to solve traffic jams. The more roads you build, the
> more car usage you trigger. In some ways the limited rate of approvals
> can be seen as acting as a natural break on the rate of patch submissions
> getting out of control. I'm not saying we've reached that point currently,
> but at some point I think it is wise to ask what is the acceptable rate
> of code churn we should try to support as a project.
> 
> 
> One of the problems of a project as large as Nova is that it is hard for
> one person to be expert at reviewing all areas of the code. As you increase
> the size of core review team we have to be careful we don't get a situation
> where we have too many patches being reviewed & approved by people who are
> not the natural experts in an area. At the same time you don't want people
> to be strictly siloed to just one area of the codebase, because you want
> people seeing the big picture. If we were grow the core review team I think
> it may be worth trying to be a bit clearer about who the experts are for
> different areas of the code, so they can focus reviews on areas where it
> would be most valuable. We do this a little informally currently, but
> I think it'd be useful to make it a bit more explicit. 

Honestly, I feel we've over metriced things to death here (not just
here, but in general). Having a lot of eager contributors, a small
number of people that understand the implications to Nova core, and a
finite time to build shared culture to make sure it's not a bag of parts
that doesn't hold together....

**it's just an algebra problem to solve.**

I get that people don't like that it takes a long time to get code into
Nova. But with so many people doing CD now, it really feels like the
cost of a false positive (landing code that never should) is way higher
than the cost of a false negative (keeping code out when it could have
come in).

I get solving algebra problems is easy, solve for X, look here is
perfect solution, add 17.3 new nova cores and everything is fixed. But
that's not the real world. Relationships matter. If they didn't we
wouldn't *vote* on new core members, we'd just auto promote people.

In my role in QA I've experienced the different cultures across
projects, the level of coherency in their core teams, and the quality of
software that comes out the other end. Honestly, Nova is doing a better
job than just about anyone else, when we quantify it on the metric I
think we all most care about: final quality. So people that aren't
steeped in that culture, coming up with ways to *fix* Nova, strikes me
as something not only a little misguided, but potentially really
dangerous second order consequences.

If you spend a lot of time in the Nova track at summit you see what that
shared culture creates. There is a lot of magic smoke in that box. Could
there be improvements, absolutely, and the most inner folks in Nova talk
about this all the time, trying to come up with new models. But I really
don't think this is a fix-it-with-math problem.

	-Sean

-- 
Sean Dague
http://dague.net

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 482 bytes
Desc: OpenPGP digital signature
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20131114/98a1f82f/attachment.pgp>