Open Stack

On 2013-10-15 17:02, Robert Collins wrote:
> On 16 October 2013 09:54, Vishvananda Ishaya <vishvananda at gmail.com> 
> wrote:
>> Hi Everyone,
>> 
>> I've been following this conversation and weighing the different 
>> sides. This is a tricky issue but I think it is important to decouple 
>> further and extend our circle of trust.
>> 
>> When nova started it was very easy to do feature development. As it 
>> has matured the pace has slowed. This is expected and necessary, but 
>> we periodically must make decoupling decisions or we will become mired 
>> in overhead. We did this already with cinder and neutron, and we have 
>> discussed doing this with virt drivers in the past.
>> 
>> We have a large number of people attempting to contribute to small 
>> sections of nova and getting frustrated with the process.  The 
>> perception of developers is much more important than the actual 
>> numbers here. If people are frustrated they are disincentivized to 
>> help and it hurts everyone. Suggesting that these contributors need to 
>> learn all of nova and help with the review queue is silly and makes us 
>> seem elitist. We should make it as easy as possible for new 
>> contributors to help.
> 
> 
> So I agree that perception is a big and significant thing here, but...
> 
> I think the fundamental issue is review latency:
> http://russellbryant.net/openstack-stats/nova-openreviews.html
> 
> Stats since the last revision without -1 or -2 (ignoring jenkins):
> 
> Average wait time: 6 days, 17 hours, 24 minutes
> 1rd quartile wait time: 0 days, 19 hours, 25 minutes
> Median wait time: 3 days, 20 hours, 3 minutes
> 3rd quartile wait time: 7 days, 7 hours, 16 minutes
> 
> At the moment 25% of the time Nova reviews sit for over a week [btw we
> probably want to not ignore Jenkins in that statistic, as I suspect
> reviewers tend to ignore patches that Jenkins hated on].
> 
> Now, you may say 'hey, 7 days isn't terrible', but actually it is: if
> a developer is producing (say) 2 patches a day, then after 7 calendar
> days they may have as many as 10 patches in a vertical queue awaiting
> review. Until the patch is reviewed for design-and-architectural
> issues (ignoring style stuff which isn't disruptive), all the work
> they are doing may need significant rework. Having to redo a week of
> work is disruptive to the contributor.
> 
> The median wait time of 3 days would nearly halve the amount of rework
> that can occur, which would be a significant benefit.
> 
> Nova has (over the last 30 days) -
> http://russellbryant.net/openstack-stats/nova-reviewers-30.txt:
> Total reviews: 3386
> Total reviewers: 173
> 
> By my count 138 of the reviewers have done less than 20 reviews in
> that 30 day period - thats less than one review a day. -
> https://docs.google.com/spreadsheet/ccc?key=0AlLkXwa7a4bpdDNjd2gtTE1odjJRYjRVWjhhR2VKQVE&usp=sharing
> 
> So, 520 reviews from that 138 folk, but if they did 1 per weekday,
> that would be 2760 reviews, bringing the total to 3386+2760-520=5626
> reviews - or nearly *double* the review bandwidth.
> 
> Now, that won't get you more cores overnight, but that sustained
> effort learning about the codebase will bring significant knowledge to
> a wide set of folk - and thats what's needed to become core, and
> increase the approval bandwidth in the team. I don't know exactly what
> russell looks for in managing the set of -core, but surely having
> enough knowledge is part of it.
> 
> Now, I've nothing against having reviewers specialise in a part of the
> code base, and making that official - but I think it must be paired
> with still requiring substantial knowledge of the projects code and
> architecture: just because something is changing in e.g.
> nova/virt/disk/api.py doesn't make it irrelevant to specific virt
> drivers, and the right way to use something in the rest of the code
> base is also relevant to virt drivers, and knowing *whats there to
> reuse* is also important.
> 
> So, I guess my proposal is, make a restricted +2 category of reviewer:
>  - social agreement to +2 only in enumerated areas
>  - still need widespread knowledge of nova's code
>  - best demonstrated by sustained regular reviews of other changes
>  - but granted after a shorter incubation period
>  - migrates to full in time
>  - privilege and responsibility lost by the same criteria as all other 
> reviewers
> 
> Of course, if this has been tried, fine - but AFAICT 'contribute
> equally' hasn't been tried yet.

FWIW, this is similar to how things work in Oslo, except that the 
restricted +2 is done through the MAINTAINERS file rather than actual 
granting of +2 authority.  So Oslo still requires a regular core 
reviewer to approve, but it only requires one because a +1 from a 
maintainer qualifies as a +2 (e.g. maintainer +1 and core +2 -> 
approved).  For Nova that might not be quite as simple as just giving +2 
to driver maintainers, but it has the advantage of keeping the people 
with an overall view of the project in the loop.

Details on the Oslo system can be found here: 
https://github.com/openstack/oslo-incubator/blob/master/MAINTAINERS#L17

-Ben