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 :| _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev