[openstack-dev] [nova] Prioritizing review of potentially "approvable" patches
mriedem at linux.vnet.ibm.com
Thu Aug 21 13:26:29 UTC 2014
On 8/21/2014 7:09 AM, Sean Dague wrote:
> FWIW, this is one of my normal morning practices, and the reason that
> that query is part of most of the gerrit dashboards -
> On 08/21/2014 06:57 AM, Daniel P. Berrange wrote:
>> Tagged with '[nova]' but this might be relevant data / idea for other
>> teams too.
>> With my code contributor hat on, one of the things that I find most the
>> frustrating about Nova code review process is that a patch can get a +2
>> vote from one core team member and then sit around for days, weeks, even
>> months without getting a second +2 vote, even if it has no negative
>> feedback at all and is a simple & important bug fix.
>> If a patch is good enough to have received one +2 vote, then compared to
>> the open patches as a whole, this patch is much more likely to be one
>> that is ready for approval & merge. It will likely be easier to review,
>> since it can be assumed other reviewers have already caught the majority
>> of the silly / tedious / time consuming bugs.
>> Letting these patches languish with a single +2 for too long makes it very
>> likely that, when a second core reviewer finally appears, there will be a
>> merge conflict or other bit-rot that will cause it to have to undergo yet
>> another rebase & re-review. This is wasting time of both our contributors
>> and our review team.
>> On this basis I suggest that core team members should consider patches
>> that already have a +2 to be high(er) priority items to review than open
>> patches as a whole.
>> Currently Nova has (on master branch)
>> - 158 patches which have at least one +2 vote, and are not approved
>> - 122 patches which have at least one +2 vote, are not approved and
>> don't have any -1 code review votes.
>> So that's 122 patches that should be easy candidates for merging right
>> now. Another 30 can possibly be merged depending on whether the core
>> reviewer agrees with the -1 feedback given or now.
>> That is way more patches than we should have outstanding in that state.
>> It is not unreasonable to say that once a patch has a single +2 vote, we
>> should aim to get either a second +2 vote or further -1 review feedback
>> in a matter of days, and certainly no longer than a week.
>> If everyone on the core team looked at the list of potentially approvable
>> patches each day I think it would significantly improve our throughput.
>> It would also decrease the amount of review work overall by reducing
>> chance that patches bitrot & need rebase for merge conflicts. And most
>> importantly of all it will give our code contributors a better impression
>> that we care about them.
>> As an added carrot, working through this list will be an effective way
>> to improve your rankings  against other core reviewers, not that I
>> mean to suggest we should care about rankings over review quality ;-P
>> The next version of gerrymander will contain a new command to allow
>> core reviewers to easily identify these patches
>> $ gerrymander todo-approvable -g nova --branch master
>> This will of course filter out patches which you yourself own since you
>> can't approve your own work. It will also filter out patches which you
>> have given feedback on already. What's left will be a list of patches
>> where you are able to apply the casting +2 vote to get to +A state.
>> If the '--strict' arg is added it will also filter out any patches which
>> have a -1 code review comment.
>>  http://russellbryant.net/openstack-stats/nova-reviewers-30.txt
>>  https://github.com/berrange/gerrymander/commit/790df913fc512580d92e808f28793e29783fecd7
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
More information about the OpenStack-dev