Open Stack

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 [1] against other core reviewers, not that I
mean to suggest we should care about rankings over review quality ;-P

The next version of gerrymander[2] 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.

Regards,
Daniel

[1] http://russellbryant.net/openstack-stats/nova-reviewers-30.txt
[2] https://github.com/berrange/gerrymander/commit/790df913fc512580d92e808f28793e29783fecd7
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|