Improving our code review efficiency

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

Improving our code review efficiency

Jon Robson
I was really happy to hear Damon, at the MediaWiki Developer Summit,
ask us how long we take to code review and whether we had communicated
a timeframe in which we promised to do it to our community. He quite
rightly stressed that this was vital for the survival of our
community. I spoke to one of our more new developers during the summit
and he also confessed to me that the reason he was an active volunteer
in our extension was that he got feedback on his code pretty quickly.

I had a few ideas about how to measure this so in my spare time I have
generated this report based on data from Gerrit patchsets using a
hacked together python script [1] which I hope will be if nothing else
an interesting artifact to talk about and generate some discussion.

Introducing:
https://www.mediawiki.org/wiki/Extension_health

To help you understand what you are reading, let's take Echo as an example:

Project: mediawiki/extensions/Echo
524 patches analysed (23 open, 501 merged)
Average review time: 29 days
Oldest open patch: (bug 41987) Updating tables indexes' names. (766
days) - https://gerrit.wikimedia.org/r/40095


The average time for code to go from submitted to merged appears to be
29 days over a dataset of 524 patches, excluding all that were written
by the L10n bot. There is a patchset there that has been _open_ for
766 days - if you look at it it was uploaded on Dec 23, 2012 12:23 PM
is -1ed by me and needs a rebase.

There are many patches like this outside Echo. We should probably be
seeing those patchsets through to completion or abandoning them on the
basis that if it hasn't been merged in over 2 years it's probably not
important and shouldn't be clogging up people's review queues and
hiding other more important patchsets which need review.

The more troubling situation is when patches have been open for some
time and have not been reviewed at all or are awaiting for some
response... let's get better at this!

Help make this ecosystem better. I will rerun the script in a month
and see if we have improved. Note our average time to review across
all those projects seems to be 18 days. That's really not good. We can
do better. We will do better.

[1] https://github.com/jdlrobson/GerritCommandLine

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Brad Jorsch (Anomie)
On Thu, Jan 29, 2015 at 12:56 PM, Jon Robson <[hidden email]> wrote:

> The average time for code to go from submitted to merged appears to be
> 29 days over a dataset of 524 patches, excluding all that were written
> by the L10n bot. There is a patchset there that has been _open_ for
> 766 days - if you look at it it was uploaded on Dec 23, 2012 12:23 PM
> is -1ed by me and needs a rebase.
>

Mean or median?

I recall talk a while back about someone else (Quim, I think?) doing this
same sort of analysis, and considering the same issues over patches that
seem to have been abandoned by their author and so on, which led to
discussions of whether we should go around abandoning patches that have
been -1ed for a long time, etc. Without proper consideration of those sorts
of issues, the statistics don't seem particularly useful.


--
Brad Jorsch (Anomie)
Software Engineer
Wikimedia Foundation
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Jon Robson
Thanks for kicking off the conversation Brad :-)

Just mean at the moment. I hacked together and I'm more than happy to
iterate on this and improve the reporting.

On the subject of patch abandonment: Personally I think we should be
abandoning inactive patches. They cause unnecessary confusion to
someone coming into a new extension wanting to help out. We may want
to change the name to 'abandon' to 'remove from code review queue' as
abandon sounds rather nasty and that's not at all what it actually
does - any abandoned patch can be restored at any time if necessary.


On Thu, Jan 29, 2015 at 1:11 PM, Brad Jorsch (Anomie)
<[hidden email]> wrote:

> On Thu, Jan 29, 2015 at 12:56 PM, Jon Robson <[hidden email]> wrote:
>
>> The average time for code to go from submitted to merged appears to be
>> 29 days over a dataset of 524 patches, excluding all that were written
>> by the L10n bot. There is a patchset there that has been _open_ for
>> 766 days - if you look at it it was uploaded on Dec 23, 2012 12:23 PM
>> is -1ed by me and needs a rebase.
>>
>
> Mean or median?
>
> I recall talk a while back about someone else (Quim, I think?) doing this
> same sort of analysis, and considering the same issues over patches that
> seem to have been abandoned by their author and so on, which led to
> discussions of whether we should go around abandoning patches that have
> been -1ed for a long time, etc. Without proper consideration of those sorts
> of issues, the statistics don't seem particularly useful.
>
>
> --
> Brad Jorsch (Anomie)
> Software Engineer
> Wikimedia Foundation
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
Jon Robson
* http://jonrobson.me.uk
* https://www.facebook.com/jonrobson
* @rakugojon

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

James Douglas
This is a situation where disciplined testing can come in really handy.

If I submit a patch, and the patch passes the tests that have been
specified for the feature it implements (or the bug it fixes), and the code
coverage is sufficiently high, then a reviewer has a running start in terms
of confidence in the correctness and completeness of the patch.

Practically speaking, this doesn't necessarily rely on rest of the project
already having a very level of code coverage; as long as there are tests
laid out for the feature in question, and the patch makes those tests pass,
it gives the code reviewer a real shot in the arm.

On Thu, Jan 29, 2015 at 1:14 PM, Jon Robson <[hidden email]> wrote:

> Thanks for kicking off the conversation Brad :-)
>
> Just mean at the moment. I hacked together and I'm more than happy to
> iterate on this and improve the reporting.
>
> On the subject of patch abandonment: Personally I think we should be
> abandoning inactive patches. They cause unnecessary confusion to
> someone coming into a new extension wanting to help out. We may want
> to change the name to 'abandon' to 'remove from code review queue' as
> abandon sounds rather nasty and that's not at all what it actually
> does - any abandoned patch can be restored at any time if necessary.
>
>
> On Thu, Jan 29, 2015 at 1:11 PM, Brad Jorsch (Anomie)
> <[hidden email]> wrote:
> > On Thu, Jan 29, 2015 at 12:56 PM, Jon Robson <[hidden email]>
> wrote:
> >
> >> The average time for code to go from submitted to merged appears to be
> >> 29 days over a dataset of 524 patches, excluding all that were written
> >> by the L10n bot. There is a patchset there that has been _open_ for
> >> 766 days - if you look at it it was uploaded on Dec 23, 2012 12:23 PM
> >> is -1ed by me and needs a rebase.
> >>
> >
> > Mean or median?
> >
> > I recall talk a while back about someone else (Quim, I think?) doing this
> > same sort of analysis, and considering the same issues over patches that
> > seem to have been abandoned by their author and so on, which led to
> > discussions of whether we should go around abandoning patches that have
> > been -1ed for a long time, etc. Without proper consideration of those
> sorts
> > of issues, the statistics don't seem particularly useful.
> >
> >
> > --
> > Brad Jorsch (Anomie)
> > Software Engineer
> > Wikimedia Foundation
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
>
>
> --
> Jon Robson
> * http://jonrobson.me.uk
> * https://www.facebook.com/jonrobson
> * @rakugojon
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Yuri Astrakhan-2
How about a simple script to create a phabricator task after a few days (a
week?) of a patch inactivity to review that patch. It will allow "assign
to", allow managers to see each dev's review queue, and will prevent
patches to fall through the cracks.

Obviously this won't be needed after we move gerrit to phabricator.

On Thu, Jan 29, 2015 at 1:44 PM, James Douglas <[hidden email]>
wrote:

> This is a situation where disciplined testing can come in really handy.
>
> If I submit a patch, and the patch passes the tests that have been
> specified for the feature it implements (or the bug it fixes), and the code
> coverage is sufficiently high, then a reviewer has a running start in terms
> of confidence in the correctness and completeness of the patch.
>
> Practically speaking, this doesn't necessarily rely on rest of the project
> already having a very level of code coverage; as long as there are tests
> laid out for the feature in question, and the patch makes those tests pass,
> it gives the code reviewer a real shot in the arm.
>
> On Thu, Jan 29, 2015 at 1:14 PM, Jon Robson <[hidden email]> wrote:
>
> > Thanks for kicking off the conversation Brad :-)
> >
> > Just mean at the moment. I hacked together and I'm more than happy to
> > iterate on this and improve the reporting.
> >
> > On the subject of patch abandonment: Personally I think we should be
> > abandoning inactive patches. They cause unnecessary confusion to
> > someone coming into a new extension wanting to help out. We may want
> > to change the name to 'abandon' to 'remove from code review queue' as
> > abandon sounds rather nasty and that's not at all what it actually
> > does - any abandoned patch can be restored at any time if necessary.
> >
> >
> > On Thu, Jan 29, 2015 at 1:11 PM, Brad Jorsch (Anomie)
> > <[hidden email]> wrote:
> > > On Thu, Jan 29, 2015 at 12:56 PM, Jon Robson <[hidden email]>
> > wrote:
> > >
> > >> The average time for code to go from submitted to merged appears to be
> > >> 29 days over a dataset of 524 patches, excluding all that were written
> > >> by the L10n bot. There is a patchset there that has been _open_ for
> > >> 766 days - if you look at it it was uploaded on Dec 23, 2012 12:23 PM
> > >> is -1ed by me and needs a rebase.
> > >>
> > >
> > > Mean or median?
> > >
> > > I recall talk a while back about someone else (Quim, I think?) doing
> this
> > > same sort of analysis, and considering the same issues over patches
> that
> > > seem to have been abandoned by their author and so on, which led to
> > > discussions of whether we should go around abandoning patches that have
> > > been -1ed for a long time, etc. Without proper consideration of those
> > sorts
> > > of issues, the statistics don't seem particularly useful.
> > >
> > >
> > > --
> > > Brad Jorsch (Anomie)
> > > Software Engineer
> > > Wikimedia Foundation
> > > _______________________________________________
> > > Wikitech-l mailing list
> > > [hidden email]
> > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> >
> >
> >
> > --
> > Jon Robson
> > * http://jonrobson.me.uk
> > * https://www.facebook.com/jonrobson
> > * @rakugojon
> >
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> >
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Brion Vibber-4
I'd like us to start by using the review request system already in gerrit
more fully.

Personally I've got a bunch of incoming reviews in my queue where I'm not
sure the current status of them or if it's wise to land them. :)

First step is probably to go through the existing old patches in
everybody's queues and either do the review, abandon the patch, or trim
down reviewers who aren't familiar with the code area.

Rejected patches should be abandoned to get them out of the queues.

Then we should go through unassigned patches more aggressively...

We also need to make sure we have default reviewers for modules, which will
be relevant also to triaging bug reports.

-- brion
On Jan 29, 2015 2:03 PM, "Yuri Astrakhan" <[hidden email]> wrote:

> How about a simple script to create a phabricator task after a few days (a
> week?) of a patch inactivity to review that patch. It will allow "assign
> to", allow managers to see each dev's review queue, and will prevent
> patches to fall through the cracks.
>
> Obviously this won't be needed after we move gerrit to phabricator.
>
> On Thu, Jan 29, 2015 at 1:44 PM, James Douglas <[hidden email]>
> wrote:
>
> > This is a situation where disciplined testing can come in really handy.
> >
> > If I submit a patch, and the patch passes the tests that have been
> > specified for the feature it implements (or the bug it fixes), and the
> code
> > coverage is sufficiently high, then a reviewer has a running start in
> terms
> > of confidence in the correctness and completeness of the patch.
> >
> > Practically speaking, this doesn't necessarily rely on rest of the
> project
> > already having a very level of code coverage; as long as there are tests
> > laid out for the feature in question, and the patch makes those tests
> pass,
> > it gives the code reviewer a real shot in the arm.
> >
> > On Thu, Jan 29, 2015 at 1:14 PM, Jon Robson <[hidden email]> wrote:
> >
> > > Thanks for kicking off the conversation Brad :-)
> > >
> > > Just mean at the moment. I hacked together and I'm more than happy to
> > > iterate on this and improve the reporting.
> > >
> > > On the subject of patch abandonment: Personally I think we should be
> > > abandoning inactive patches. They cause unnecessary confusion to
> > > someone coming into a new extension wanting to help out. We may want
> > > to change the name to 'abandon' to 'remove from code review queue' as
> > > abandon sounds rather nasty and that's not at all what it actually
> > > does - any abandoned patch can be restored at any time if necessary.
> > >
> > >
> > > On Thu, Jan 29, 2015 at 1:11 PM, Brad Jorsch (Anomie)
> > > <[hidden email]> wrote:
> > > > On Thu, Jan 29, 2015 at 12:56 PM, Jon Robson <[hidden email]>
> > > wrote:
> > > >
> > > >> The average time for code to go from submitted to merged appears to
> be
> > > >> 29 days over a dataset of 524 patches, excluding all that were
> written
> > > >> by the L10n bot. There is a patchset there that has been _open_ for
> > > >> 766 days - if you look at it it was uploaded on Dec 23, 2012 12:23
> PM
> > > >> is -1ed by me and needs a rebase.
> > > >>
> > > >
> > > > Mean or median?
> > > >
> > > > I recall talk a while back about someone else (Quim, I think?) doing
> > this
> > > > same sort of analysis, and considering the same issues over patches
> > that
> > > > seem to have been abandoned by their author and so on, which led to
> > > > discussions of whether we should go around abandoning patches that
> have
> > > > been -1ed for a long time, etc. Without proper consideration of those
> > > sorts
> > > > of issues, the statistics don't seem particularly useful.
> > > >
> > > >
> > > > --
> > > > Brad Jorsch (Anomie)
> > > > Software Engineer
> > > > Wikimedia Foundation
> > > > _______________________________________________
> > > > Wikitech-l mailing list
> > > > [hidden email]
> > > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> > >
> > >
> > >
> > > --
> > > Jon Robson
> > > * http://jonrobson.me.uk
> > > * https://www.facebook.com/jonrobson
> > > * @rakugojon
> > >
> > > _______________________________________________
> > > Wikitech-l mailing list
> > > [hidden email]
> > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> > >
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> >
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Yuri Astrakhan-2
Brion, i would love to use gerrit more fully (that is until we finally
migrate! :)), but gerrit to my knowledge does not differentiate between a
CC (review if you want to) and TO (i want you to +2). Having multiple cooks
means some patches don't get merged at all.  I feel each patch should be
assigned to a person who will +2 it.  This does not preclude others from
+2ing it, but it designates one responsible for the answer.

On Thu, Jan 29, 2015 at 2:33 PM, Brion Vibber <[hidden email]> wrote:

> I'd like us to start by using the review request system already in gerrit
> more fully.
>
> Personally I've got a bunch of incoming reviews in my queue where I'm not
> sure the current status of them or if it's wise to land them. :)
>
> First step is probably to go through the existing old patches in
> everybody's queues and either do the review, abandon the patch, or trim
> down reviewers who aren't familiar with the code area.
>
> Rejected patches should be abandoned to get them out of the queues.
>
> Then we should go through unassigned patches more aggressively...
>
> We also need to make sure we have default reviewers for modules, which will
> be relevant also to triaging bug reports.
>
> -- brion
> On Jan 29, 2015 2:03 PM, "Yuri Astrakhan" <[hidden email]>
> wrote:
>
> > How about a simple script to create a phabricator task after a few days
> (a
> > week?) of a patch inactivity to review that patch. It will allow "assign
> > to", allow managers to see each dev's review queue, and will prevent
> > patches to fall through the cracks.
> >
> > Obviously this won't be needed after we move gerrit to phabricator.
> >
> > On Thu, Jan 29, 2015 at 1:44 PM, James Douglas <[hidden email]>
> > wrote:
> >
> > > This is a situation where disciplined testing can come in really handy.
> > >
> > > If I submit a patch, and the patch passes the tests that have been
> > > specified for the feature it implements (or the bug it fixes), and the
> > code
> > > coverage is sufficiently high, then a reviewer has a running start in
> > terms
> > > of confidence in the correctness and completeness of the patch.
> > >
> > > Practically speaking, this doesn't necessarily rely on rest of the
> > project
> > > already having a very level of code coverage; as long as there are
> tests
> > > laid out for the feature in question, and the patch makes those tests
> > pass,
> > > it gives the code reviewer a real shot in the arm.
> > >
> > > On Thu, Jan 29, 2015 at 1:14 PM, Jon Robson <[hidden email]>
> wrote:
> > >
> > > > Thanks for kicking off the conversation Brad :-)
> > > >
> > > > Just mean at the moment. I hacked together and I'm more than happy to
> > > > iterate on this and improve the reporting.
> > > >
> > > > On the subject of patch abandonment: Personally I think we should be
> > > > abandoning inactive patches. They cause unnecessary confusion to
> > > > someone coming into a new extension wanting to help out. We may want
> > > > to change the name to 'abandon' to 'remove from code review queue' as
> > > > abandon sounds rather nasty and that's not at all what it actually
> > > > does - any abandoned patch can be restored at any time if necessary.
> > > >
> > > >
> > > > On Thu, Jan 29, 2015 at 1:11 PM, Brad Jorsch (Anomie)
> > > > <[hidden email]> wrote:
> > > > > On Thu, Jan 29, 2015 at 12:56 PM, Jon Robson <[hidden email]>
> > > > wrote:
> > > > >
> > > > >> The average time for code to go from submitted to merged appears
> to
> > be
> > > > >> 29 days over a dataset of 524 patches, excluding all that were
> > written
> > > > >> by the L10n bot. There is a patchset there that has been _open_
> for
> > > > >> 766 days - if you look at it it was uploaded on Dec 23, 2012 12:23
> > PM
> > > > >> is -1ed by me and needs a rebase.
> > > > >>
> > > > >
> > > > > Mean or median?
> > > > >
> > > > > I recall talk a while back about someone else (Quim, I think?)
> doing
> > > this
> > > > > same sort of analysis, and considering the same issues over patches
> > > that
> > > > > seem to have been abandoned by their author and so on, which led to
> > > > > discussions of whether we should go around abandoning patches that
> > have
> > > > > been -1ed for a long time, etc. Without proper consideration of
> those
> > > > sorts
> > > > > of issues, the statistics don't seem particularly useful.
> > > > >
> > > > >
> > > > > --
> > > > > Brad Jorsch (Anomie)
> > > > > Software Engineer
> > > > > Wikimedia Foundation
> > > > > _______________________________________________
> > > > > Wikitech-l mailing list
> > > > > [hidden email]
> > > > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> > > >
> > > >
> > > >
> > > > --
> > > > Jon Robson
> > > > * http://jonrobson.me.uk
> > > > * https://www.facebook.com/jonrobson
> > > > * @rakugojon
> > > >
> > > > _______________________________________________
> > > > Wikitech-l mailing list
> > > > [hidden email]
> > > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> > > >
> > > _______________________________________________
> > > Wikitech-l mailing list
> > > [hidden email]
> > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> > >
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Brion Vibber-4
Good point Yuri -- a lot of those items on my queue are assigned to several
reviewers so none of us feels ownership, and that's definitely part of the
reason some of them sit around so long.

A regular bot run that assigns untouched review requests to a single person
in Phab probably does make sense...

-- brion

On Thu, Jan 29, 2015 at 2:48 PM, Yuri Astrakhan <[hidden email]>
wrote:

> Brion, i would love to use gerrit more fully (that is until we finally
> migrate! :)), but gerrit to my knowledge does not differentiate between a
> CC (review if you want to) and TO (i want you to +2). Having multiple cooks
> means some patches don't get merged at all.  I feel each patch should be
> assigned to a person who will +2 it.  This does not preclude others from
> +2ing it, but it designates one responsible for the answer.
>
> On Thu, Jan 29, 2015 at 2:33 PM, Brion Vibber <[hidden email]>
> wrote:
>
> > I'd like us to start by using the review request system already in gerrit
> > more fully.
> >
> > Personally I've got a bunch of incoming reviews in my queue where I'm not
> > sure the current status of them or if it's wise to land them. :)
> >
> > First step is probably to go through the existing old patches in
> > everybody's queues and either do the review, abandon the patch, or trim
> > down reviewers who aren't familiar with the code area.
> >
> > Rejected patches should be abandoned to get them out of the queues.
> >
> > Then we should go through unassigned patches more aggressively...
> >
> > We also need to make sure we have default reviewers for modules, which
> will
> > be relevant also to triaging bug reports.
> >
> > -- brion
> > On Jan 29, 2015 2:03 PM, "Yuri Astrakhan" <[hidden email]>
> > wrote:
> >
> > > How about a simple script to create a phabricator task after a few days
> > (a
> > > week?) of a patch inactivity to review that patch. It will allow
> "assign
> > > to", allow managers to see each dev's review queue, and will prevent
> > > patches to fall through the cracks.
> > >
> > > Obviously this won't be needed after we move gerrit to phabricator.
> > >
> > > On Thu, Jan 29, 2015 at 1:44 PM, James Douglas <[hidden email]
> >
> > > wrote:
> > >
> > > > This is a situation where disciplined testing can come in really
> handy.
> > > >
> > > > If I submit a patch, and the patch passes the tests that have been
> > > > specified for the feature it implements (or the bug it fixes), and
> the
> > > code
> > > > coverage is sufficiently high, then a reviewer has a running start in
> > > terms
> > > > of confidence in the correctness and completeness of the patch.
> > > >
> > > > Practically speaking, this doesn't necessarily rely on rest of the
> > > project
> > > > already having a very level of code coverage; as long as there are
> > tests
> > > > laid out for the feature in question, and the patch makes those tests
> > > pass,
> > > > it gives the code reviewer a real shot in the arm.
> > > >
> > > > On Thu, Jan 29, 2015 at 1:14 PM, Jon Robson <[hidden email]>
> > wrote:
> > > >
> > > > > Thanks for kicking off the conversation Brad :-)
> > > > >
> > > > > Just mean at the moment. I hacked together and I'm more than happy
> to
> > > > > iterate on this and improve the reporting.
> > > > >
> > > > > On the subject of patch abandonment: Personally I think we should
> be
> > > > > abandoning inactive patches. They cause unnecessary confusion to
> > > > > someone coming into a new extension wanting to help out. We may
> want
> > > > > to change the name to 'abandon' to 'remove from code review queue'
> as
> > > > > abandon sounds rather nasty and that's not at all what it actually
> > > > > does - any abandoned patch can be restored at any time if
> necessary.
> > > > >
> > > > >
> > > > > On Thu, Jan 29, 2015 at 1:11 PM, Brad Jorsch (Anomie)
> > > > > <[hidden email]> wrote:
> > > > > > On Thu, Jan 29, 2015 at 12:56 PM, Jon Robson <
> [hidden email]>
> > > > > wrote:
> > > > > >
> > > > > >> The average time for code to go from submitted to merged appears
> > to
> > > be
> > > > > >> 29 days over a dataset of 524 patches, excluding all that were
> > > written
> > > > > >> by the L10n bot. There is a patchset there that has been _open_
> > for
> > > > > >> 766 days - if you look at it it was uploaded on Dec 23, 2012
> 12:23
> > > PM
> > > > > >> is -1ed by me and needs a rebase.
> > > > > >>
> > > > > >
> > > > > > Mean or median?
> > > > > >
> > > > > > I recall talk a while back about someone else (Quim, I think?)
> > doing
> > > > this
> > > > > > same sort of analysis, and considering the same issues over
> patches
> > > > that
> > > > > > seem to have been abandoned by their author and so on, which led
> to
> > > > > > discussions of whether we should go around abandoning patches
> that
> > > have
> > > > > > been -1ed for a long time, etc. Without proper consideration of
> > those
> > > > > sorts
> > > > > > of issues, the statistics don't seem particularly useful.
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Brad Jorsch (Anomie)
> > > > > > Software Engineer
> > > > > > Wikimedia Foundation
> > > > > > _______________________________________________
> > > > > > Wikitech-l mailing list
> > > > > > [hidden email]
> > > > > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Jon Robson
> > > > > * http://jonrobson.me.uk
> > > > > * https://www.facebook.com/jonrobson
> > > > > * @rakugojon
> > > > >
> > > > > _______________________________________________
> > > > > Wikitech-l mailing list
> > > > > [hidden email]
> > > > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> > > > >
> > > > _______________________________________________
> > > > Wikitech-l mailing list
> > > > [hidden email]
> > > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> > > >
> > > _______________________________________________
> > > Wikitech-l mailing list
> > > [hidden email]
> > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
> >
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

MZMcBride-2
Brion Vibber wrote:
>Good point Yuri -- a lot of those items on my queue are assigned to
>several reviewers so none of us feels ownership, and that's definitely
>part of the reason some of them sit around so long.
>
>A regular bot run that assigns untouched review requests to a single
>person in Phab probably does make sense...

Many Gerrit changesets already have an associated Phabricator task, of
course. I'm wary of trying to tear down one queue by building another.

For Bugzilla (now Phabricator), we ended up creating a full-time position
that focuses on reviewing, triaging, and generally handling bugs (now
tasks). While I'm not still not sure I agree with this approach, a Review
Wrangler or similar wouldn't be without precedent.

MZMcBride



_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Quim Gil-2
In reply to this post by Jon Robson
On Thu, Jan 29, 2015 at 12:56 PM, Jon Robson <[hidden email]> wrote:

>
> Introducing:
> https://www.mediawiki.org/wiki/Extension_health



Interesting! I'm hopping between flights back to Europe, and I don't have
time to review these metrics more carefully, but please check
http://korma.wmflabs.org/browser/gerrit_review_queue.html and let me know
what you miss.

--
Quim Gil
Engineering Community Manager @ Wikimedia Foundation
http://www.mediawiki.org/wiki/User:Qgil
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Jon Robson
As I mentioned to Nemo on the talk page, I want an easy way to see how
my code review efficiency compares to other projects and to see which
projects are getting more love than others. A few thoughts:

1) From http://korma.wmflabs.org/browser/repository.html?repository=gerrit.wikimedia.org_mediawiki_extensions_MobileFrontend
I can see it takes 3.2 days to get a review (I think - there are too
many numbers to look at and no key to tell the difference)
I can see on Echo
http://korma.wmflabs.org/browser/repository.html?repository=gerrit.wikimedia.org_mediawiki_extensions_Echo
it is 10.7 days but want I really want is to see a league table type
thing to tell where we are giving more attention compared to other
projects.

2) Also I think an average review time is only really useful if it is
based on data from the last month.

3) What about open patchsets - does average review time take into
account that some patches still haven't got merged? If a patch has
been sitting around for 100 days, I care more about this then an
existing patch that got merged after 5 days. These should impact the
average.

4) Also this dashboard is not actionable and has no call to action -
why don't we show the most neglected patchsets on each page and
encourage people to actually go and review them!

On Thu, Jan 29, 2015 at 3:38 PM, Quim Gil <[hidden email]> wrote:

> On Thu, Jan 29, 2015 at 12:56 PM, Jon Robson <[hidden email]> wrote:
>
>>
>> Introducing:
>> https://www.mediawiki.org/wiki/Extension_health
>
>
>
> Interesting! I'm hopping between flights back to Europe, and I don't have
> time to review these metrics more carefully, but please check
> http://korma.wmflabs.org/browser/gerrit_review_queue.html and let me know
> what you miss.
>
> --
> Quim Gil
> Engineering Community Manager @ Wikimedia Foundation
> http://www.mediawiki.org/wiki/User:Qgil
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
Jon Robson
* http://jonrobson.me.uk
* https://www.facebook.com/jonrobson
* @rakugojon

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Daniel Friesen-2
In reply to this post by Jon Robson
On 2015-01-29 1:14 PM, Jon Robson wrote:

> Thanks for kicking off the conversation Brad :-)
>
> Just mean at the moment. I hacked together and I'm more than happy to
> iterate on this and improve the reporting.
>
> On the subject of patch abandonment: Personally I think we should be
> abandoning inactive patches. They cause unnecessary confusion to
> someone coming into a new extension wanting to help out. We may want
> to change the name to 'abandon' to 'remove from code review queue' as
> abandon sounds rather nasty and that's not at all what it actually
> does - any abandoned patch can be restored at any time if necessary.
Unfortunately, under Gerrit, abandoning a patch puts "inactive, restore
if you can finish it" patches in the same bin as "this was a complete
failure".

Not only do you have to examine each abandoned patch individually to see
if it's worth reopening, but after they leave your recently closed list
they are all segregated to an obscure place not everyone knows how to
get to (you have to manually search for "owner:self status:abandoned").

Proper handling of 'remove from code review queue' abandonment should
include a section on a user's dashboard listing patches that have been
removed from the queue due to inactivity, etc... but not outright rejected.

~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Federico Leva (Nemo)
The "remove from code review queue" is not that hard really, you can
just remove yourself (and reviewers you added) from reviewers. The most
helpful reviewers also comment on why they've removed themselves. If
reviewers removed themselves from patches they have no intention
whatsoever to review (which is fine), things would be much easier.

Other effective ways to effectively remove an open patch from the review
workflows (and from the korma stats, though not from [[Gerrit/Reports]])
is to self-give a -1 and/or slap a "[WIP]" in the first line.

Nemo

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Florian Schmidt
But this doesn't remove it from the projects review queue (or search queries, if you just use "project:mediawiki/extensions/MobileFrontend status:open").

Kind regards,
Florian

-----Urspr√ľngliche Nachricht-----
Von: [hidden email] [mailto:[hidden email]] Im Auftrag von Federico Leva (Nemo)
Gesendet: Freitag, 30. Januar 2015 08:52
An: [hidden email]
Betreff: Re: [Wikitech-l] Improving our code review efficiency

The "remove from code review queue" is not that hard really, you can just remove yourself (and reviewers you added) from reviewers. The most helpful reviewers also comment on why they've removed themselves. If reviewers removed themselves from patches they have no intention whatsoever to review (which is fine), things would be much easier.

Other effective ways to effectively remove an open patch from the review workflows (and from the korma stats, though not from [[Gerrit/Reports]]) is to self-give a -1 and/or slap a "[WIP]" in the first line.

Nemo

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Federico Leva (Nemo)
 > But this doesn't remove it from the projects review queue (or search
 > queries, if you just use "project:mediawiki/extensions/MobileFrontend
 > status:open").

IMHO the distinction between cleaning up personal and global dashboards
is not so important. In the end, code review is performed by
individuals. If each reviewer has a more relevant review queue, all
reviewers will be more efficient and the backlog will decrease for everyone.

After all, we see from
https://www.mediawiki.org/wiki/Gerrit/Reports/Code_review_activity that
20 reviewers do 50 % of the merging in gerrit. Some queries like those
listed at https://www.mediawiki.org/wiki/Gerrit/Navigation show that
over ten users have over 100 review requests in their dashboards, among
those who merged something in the last month. I doubt that's efficient
for them.

Nemo

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Improving our code review efficiency

Quim Gil-2
What about a Gerrit Cleanup Day involving all Wikimedia Foundation
developers and whoever else wants to be involved?

Feedback welcome: https://phabricator.wikimedia.org/T88531

--
Quim Gil
Engineering Community Manager @ Wikimedia Foundation
http://www.mediawiki.org/wiki/User:Qgil
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l