Open Stack

Le 21/08/2014 13:57, Daniel P. Berrange a écrit :
> 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

Strong +1 here for 2 reasons :
  - We make sure the taken direction is agreed for at least 2 people
  - When it goes to the gate, there is less risk to have a merge failed

That thread makes me think about a blogpost I just read about code 
reviews, really worth it :
http://swreflections.blogspot.fr/2014/08/dont-waste-time-on-code-reviews.html

-Sylvain