settingsLogin | Registersettings

[openstack-dev] [nova] Prioritizing review of potentially "approvable" patches

0 votes

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 :|

asked Aug 21, 2014 in openstack-dev by Daniel_P._Berrange (27,920 points)   3 4 10
retagged Jan 28, 2015 by admin

5 Responses

0 votes

FWIW, this is one of my normal morning practices, and the reason that
that query is part of most of the gerrit dashboards -
https://github.com/stackforge/gerrit-dash-creator/blob/master/dashboards/compute-program.dash

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 [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

--
Sean Dague
http://dague.net

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 482 bytes
Desc: OpenPGP digital signature
URL:

responded Aug 21, 2014 by Sean_Dague (66,200 points)   4 11 20
0 votes

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 -
https://github.com/stackforge/gerrit-dash-creator/blob/master/dashboards/compute-program.dash

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 [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


OpenStack-dev mailing list
OpenStack-dev at lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Yeah:

https://review.openstack.org/#/projects/openstack/nova,dashboards/important-changes:review-inbox-dashboard

--

Thanks,

Matt Riedemann

responded Aug 21, 2014 by Matt_Riedemann (48,320 points)   3 10 26
0 votes

On Thu, Aug 21, 2014 at 08:26:29AM -0500, Matt Riedemann wrote:

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 -
https://github.com/stackforge/gerrit-dash-creator/blob/master/dashboards/compute-program.dash

https://review.openstack.org/#/projects/openstack/nova,dashboards/important-changes:review-inbox-dashboard

That should really sort the changes so oldest one is shown first rather
than most recently changed one, otherwise stuff that is waiting the
longest is least likely to be seen & processed - particularly as it is
truncating the list at 50 changes and we have 100+ pending. It ought to
filter out ones with a +A on them already too

Regards,
Daniel
--
|: 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 :|

responded Aug 21, 2014 by Daniel_P._Berrange (27,920 points)   3 4 10
0 votes

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

responded Aug 21, 2014 by Sylvain_Bauza (14,100 points)   1 3 6
0 votes

Hear, hear!

Dan

On 08/21/2014 07: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 [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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3449 bytes
Desc: S/MIME Cryptographic Signature
URL:

responded Aug 21, 2014 by Dan_Genin (820 points)   1 2
...