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 <[email protected]> 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 <[email protected]> > 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" <[email protected]> > > 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 <[email protected] > > > > > 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 <[email protected]> > > 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) > > > > > <[email protected]> wrote: > > > > > > On Thu, Jan 29, 2015 at 12:56 PM, Jon Robson < > [email protected]> > > > > > 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 > > > > > > [email protected] > > > > > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l > > > > > > > > > > > > > > > > > > > > -- > > > > > Jon Robson > > > > > * http://jonrobson.me.uk > > > > > * https://www.facebook.com/jonrobson > > > > > * @rakugojon > > > > > > > > > > _______________________________________________ > > > > > Wikitech-l mailing list > > > > > [email protected] > > > > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l > > > > > > > > > _______________________________________________ > > > > Wikitech-l mailing list > > > > [email protected] > > > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l > > > > > > > _______________________________________________ > > > Wikitech-l mailing list > > > [email protected] > > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l > > _______________________________________________ > > Wikitech-l mailing list > > [email protected] > > https://lists.wikimedia.org/mailman/listinfo/wikitech-l > > > _______________________________________________ > Wikitech-l mailing list > [email protected] > https://lists.wikimedia.org/mailman/listinfo/wikitech-l > _______________________________________________ Wikitech-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikitech-l
