Re: [openstack-dev] [nova] Prioritizing review of potentially approvable patches
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 signature.asc Description: OpenPGP digital signature ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Prioritizing review of potentially approvable patches
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@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 ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Prioritizing review of potentially approvable patches
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 :| ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Prioritizing review of potentially approvable patches
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 ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Prioritizing review of potentially approvable patches
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 smime.p7s Description: S/MIME Cryptographic Signature ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
