Hi, I find that a lot of contributors(myself included) get discouraged due to having their patches rot in the review queue. It's not fair to the contributors have a patch rot without any feedback. It's also not great to r+ patch a month after it was submitted.
I propose a two step solution to this
1) Every bugzilla account should have bugzilla whine(https://bugzilla.mozilla.org/editwhines.cgi or equivalent) turned on to remind reviewers about outstanding r? requests.
2) Institute a policy where reviewers have to reassign r? to someone else if they know they wont be able to review something within a week. It might turn into a game of hot potato, or it might end up with r? flags being removed from attachments. I think that's better than quietly ignoring the problem.
I realize that some people do a lot more reviews than others, but letting patches languish in the queue doesn't make those queues smaller. We should have more fallback reviewers instead.
> Hi, > I find that a lot of contributors(myself included) get discouraged due > to having their patches rot in the review queue. > It's not fair to the contributors have a patch rot without any feedback. > It's also not great to r+ patch a month after it was submitted.
> I propose a two step solution to this
> 1) Every bugzilla account should have bugzilla > whine(https://bugzilla.mozilla.org/editwhines.cgi or equivalent) turned > on to remind reviewers about outstanding r? requests.
Yes, I'd like to get reminded if I haven't reviewed a patch in a week or so.
> 2) Institute a policy where reviewers have to reassign r? to someone > else if they know they wont be able to review something within a week. > It might turn into a game of hot potato, or it might end up with r? > flags being removed from attachments. I think that's better than quietly > ignoring the problem.
Sounds ok to me.
> I realize that some people do a lot more reviews than others, but > letting patches languish in the queue doesn't make those queues smaller. > We should have more fallback reviewers instead.
I do quite a few reviews, and I'd like to have tools to help my work. One thing is to get reminders.
Hmm, I have bz_canusewhines. Why haven't I created such reminder yet :/
On Thu, Aug 11, 2011 at 9:33 AM, Taras Glek <tg...@mozilla.com> wrote: > I find that a lot of contributors(myself included) get discouraged due to > having their patches rot in the review queue. > It's not fair to the contributors have a patch rot without any feedback. > It's also not great to r+ patch a month after it was submitted.
Are patches accidentally falling through the cracks, or being deliberately ignored?
If the latter, I think the only effective approach is to tackle the delinquent reviewers directly. If one-on-one engagement doesn't work, escalate.
Rob -- "If we claim to be without sin, we deceive ourselves and the truth is not in us. If we confess our sins, he is faithful and just and will forgive us our sins and purify us from all unrighteousness. If we claim we have not sinned, we make him out to be a liar and his word is not in us." [1 John 1:8-10]
> Hi, > I find that a lot of contributors(myself included) get discouraged due > to having their patches rot in the review queue. > It's not fair to the contributors have a patch rot without any feedback. > It's also not great to r+ patch a month after it was submitted.
> I propose a two step solution to this
One of the things I started doing at the beginning of the year was having an aim of getting reviews in the Thunderbird product down to something like an average of 2 weeks, and MailNews Core to 3 weeks (iirc).
Unfortunately this effort has been sidetracked a bit with me getting lots of other things to do, however, I think it has reasonable success.
We started sometime towards the end of last year with patches that had been in the review queue for several years. We looked at those, we review and landed patches if appropriate (sometimes just finishing the work ourselves rather than waiting to see if the original contributor was still around); we cancelled/denied reviews if a new patch was required etc.
Once we'd gotten rid of the majority of the backlog, we then started pushing reviewers to get patches reviewed quickly, at one point we almost hit 0 outstanding review requests.
Recently we've had more turn over of patches and reviews, and it is holiday time, so we've got more in progress at any one time, but without re-running the stats I'm guessing most patches are generally reviewed within 2-3 weeks at the outside, and probably averaging a week. Our oldest request in Thunderbird product is currently from the mid July, which is way better than the several years we used to have.
One of the favourite ones for clearing old reviews was looking at open review requests on closed bugs - if the bug is closed, why is it there... (I even have a whine set up for the products I watch).
The advantage of getting rid of the backlog first was that it gave us a much clearer view of the more recent reviews and we can easily see when some are getting left behind.
We haven't been so successful in mailnews core yet as that had a larger backlog and we're still trying to get that sorted out.
> 1) Every bugzilla account should have bugzilla > whine(https://bugzilla.mozilla.org/editwhines.cgi or equivalent) turned > on to remind reviewers about outstanding r? requests.
I think that's reasonable, but I don't think that stops people with a large review queue from ignoring it - getting a whine email every week (say) doesn't mean you'll do something about it.
Generally I think reviewers need to be getting into a habit of looking at their queues on a frequent basis. Obviously some methods will work better for some folks than others.
> 2) Institute a policy where reviewers have to reassign r? to someone > else if they know they wont be able to review something within a week. > It might turn into a game of hot potato, or it might end up with r? > flags being removed from attachments. I think that's better than quietly > ignoring the problem.
One of the problems with bugzilla with this approach is that you then loose easy visibility when review on the patch was first requested, and how long that's been going on (for example, My Reviews, just shows the date the request was created/reassigned, not when the flag was first requested).
> I realize that some people do a lot more reviews than others, but > letting patches languish in the queue doesn't make those queues smaller. > We should have more fallback reviewers instead.
I think what we need to have is module owners/team leads frequently looking at queues and what's happening with them. If patches are getting left behind, why is that, which of course leads onto do we need to reassign those reviews to someone else or do we need to get more reviewers.
I was recently told of a company that escalates any expense report that hasn't been approved in 24 hours to the next level of management up. It might be helpful to something similar in bugzilla. If a review hasn't been processed in say a week, bugzilla could automatically re-assign it to the module owner (based on the product and component).
The problem I'm trying to solve here is reviews requested of people who are traveling, on PTO, paternity/maternity leave etc. and thus not paying attention to bugzilla.
> I was recently told of a company that escalates any expense report that > hasn't been approved in 24 hours to the next level of management up. It > might be helpful to something similar in bugzilla. If a review hasn't > been processed in say a week, bugzilla could automatically re-assign it > to the module owner (based on the product and component).
> The problem I'm trying to solve here is reviews requested of people who > are traveling, on PTO, paternity/maternity leave etc. and thus not > paying attention to bugzilla.
I'm not sure that would help; it's been my experience (particularly in dusty corners of gecko core) that the module owners are the people who don't do their reviews.
> On Thu, Aug 11, 2011 at 9:33 AM, Taras Glek<tg...@mozilla.com> wrote:
>> I find that a lot of contributors(myself included) get discouraged due to >> having their patches rot in the review queue. >> It's not fair to the contributors have a patch rot without any feedback. >> It's also not great to r+ patch a month after it was submitted.
> Are patches accidentally falling through the cracks, or being deliberately > ignored?
> If the latter, I think the only effective approach is to tackle the > delinquent reviewers directly. If one-on-one engagement doesn't work, > escalate.
I think it's a bit of both. We need to make it easier to keep track of own review queue and to find people with delinquent reviews.
Perhaps someone can cook up a bugzilla search to track stale r? requests and publicize that on some mailing list?
"Developer XYZ hasn't reviewed bug #### in a week" -> then someone else could pick up the review.
> On 08/10/2011 09:35 PM, Robert O'Callahan wrote: >> On Thu, Aug 11, 2011 at 9:33 AM, Taras Glek<tg...@mozilla.com> wrote:
>>> I find that a lot of contributors(myself included) get discouraged due to >>> having their patches rot in the review queue. >>> It's not fair to the contributors have a patch rot without any feedback. >>> It's also not great to r+ patch a month after it was submitted.
>> Are patches accidentally falling through the cracks, or being deliberately >> ignored?
>> If the latter, I think the only effective approach is to tackle the >> delinquent reviewers directly. If one-on-one engagement doesn't work, >> escalate.
> I think it's a bit of both. We need to make it easier to keep track of own review queue and to find people with delinquent reviews.
> Perhaps someone can cook up a bugzilla search to track stale r? requests and publicize that on some mailing list?
> "Developer XYZ hasn't reviewed bug #### in a week" -> then someone else could pick up the review.
FWIW I think it would be pretty trivial to write an extension to do the email whine by default (with cc'ing a mailing list or whatever) followed by reassigning the review via a per-module list of prioritized fallback reviewers.
Sounds like a good intern or new contributor project to me (assuming that this is a real problem, I don't see any data presented to back up the assertion yet).
On Aug 11, 10:26 am, Taras Glek <tg...@mozilla.com> wrote:
> Perhaps someone can cook up a bugzilla search to track stale r? requests > and publicize that on some mailing list?
> "Developer XYZ hasn't reviewed bug #### in a week" -> then someone else > could pick up the review.
That's a good idea. A good portion of the patches that sit in my review queue are patches to code that no one really "owns" - I ended up being picked as a reviewer because I'm a peer of the module that the code is nominally part of, not because I'm particularly familiar with the code. These never get very high on the priority list because they're usually not very important fixes, and performing the review requires a lot of familiarizing yourself with old/unknown code. A more formalized way to request review "from the wind" for a given patch would let me indicate, by forwarding the request, that anyone who feels like reviewing the patch should feel free to (even if they're not officially "peers") - I might want to double check the review after the fact, but it doesn't need to block on me in particular. That would also be an effective way to develop ownership of unowned areas of code.
> I'm not sure that would help; it's been my experience (particularly in > dusty corners of gecko core) that the module owners are the people who > don't do their reviews.
> zw
That, IMO, is a another problem to be solved. Absentee module owners should be replaced.
> That, IMO, is a another problem to be solved. Absentee module owners > should be replaced.
How much time are we expecting module owners to spend on reviews? How much time do reviews take?
My data from my personal experience is that I tend to have lots of review requests that are more than a week old in my queue, and that I'm spending at least 50% of my work time on reviews, fwiw.
On Wed, 10 Aug 2011 14:33:18 -0700, Taras Glek wrote: > Hi, > I find that a lot of contributors(myself included) get discouraged due > to having their patches rot in the review queue. > It's not fair to the contributors have a patch rot without any feedback. > It's also not great to r+ patch a month after it was submitted.
> I propose a two step solution to this
> 1) Every bugzilla account should have bugzilla > whine(https://bugzilla.mozilla.org/editwhines.cgi or equivalent) turned > on to remind reviewers about outstanding r? requests.
> 2) Institute a policy where reviewers have to reassign r? to someone > else if they know they wont be able to review something within a week. > It might turn into a game of hot potato, or it might end up with r? > flags being removed from attachments. I think that's better than quietly > ignoring the problem.
> I realize that some people do a lot more reviews than others, but > letting patches languish in the queue doesn't make those queues smaller. > We should have more fallback reviewers instead.
How would you deal with people like, um, timeless, with review requests going back to 2004.
On Thu, Aug 11, 2011 at 2:14 PM, Brad Lassey <blas...@mozilla.com> wrote: > That, IMO, is a another problem to be solved. Absentee module owners should > be replaced.
With...
If there were more-suitable module owners around, they'd probably already be the module owner, right? If there's a module owner that's not keeping up with their reviews, and you offer to take over the module from them, I bet they'll kiss you.
> If there were more-suitable module owners around, they'd probably > already be the module owner, right? If there's a module owner that's > not keeping up with their reviews, and you offer to take over the > module from them, I bet they'll kiss you.
More saying that we should make an effort to find suitable module owners (or active peers to pick up the slack) for modules that are falling behind on reviews. It could be the case that absentee module owners aren't aware that others view them as absentee.
Also, my goal for re-assigning reviews to module owners isn't to make module owners do the reviews, but to rather give them the responsibility to find someone to do the reviews.
Wow, some impressive requester/requestee combinations around near the top... people who aren't working on Mozilla stuff any more, etc. And I wonder how many patches even still apply somewhat.
Robert Kaiser
-- Note that any statements of mine - no matter how passionate - are never meant to be offensive but very often as food for thought or possible arguments that we as a community should think about. And most of the time, I even appreciate irony and fun! :)
> How would you deal with people like, um, timeless, with review requests > going back to 2004.
Timeless at least tends to still be around somewhat - and then, I don't see a lot of requests *to* him, just *from* him, the oldest one where he's the requestee is 2006, and it's a rare case. The other way round isn't. People like leaf and a number of others are much more of a problem, as they don't work on Mozilla stuff any more and haven't been seen around anywhere for a long time.
Robert Kaiser
-- Note that any statements of mine - no matter how passionate - are never meant to be offensive but very often as food for thought or possible arguments that we as a community should think about. And most of the time, I even appreciate irony and fun! :)
On Thursday 2011-08-11 10:01 -0700, Zack Weinberg wrote:
> I'm not sure that would help; it's been my experience (particularly > in dusty corners of gecko core) that the module owners are the > people who don't do their reviews.
Yeah, that's probably me. I've been spending as much time as I can on reviews lately (it's my primary job right now; I basically haven't been writing code), but there's also a limit to the number of hours a day I can spend doing code reviews and maintain my sanity.
The stuff in my review queue is really a mix of things (to use some extreme examples):
* patches with well-defined goals that the author is, with good reason, reasonably confident in
* like above, except from new contributors who need much more careful review and mentoring
* patches that redesign a subsystem without previous discussion with the other maintainers of the code (of, e.g., what the constraints that led to the current design are or what other changes need to happen soon in that area of code), in other words, where the review request essentially requests either rubber-stamping of the new design, or that a replacement design be offered (which is a lot of work)
* patches that fix the case that the patch author cares about but break a lot of others -- in other words, patches where the author has done the first 20% of the work, is asking me to do the next 50%, and is quite willing to do the typing at the end to finish the last 30%
These aren't really clear divisions: many patches are in between these extremes.
Should things in or near the latter two categories be treated the same as things in the former categories? Should whether I think the patch is important (or whether I think it's even worth the time being spent on it) influence how quickly I review patches in any or all of these categories? I tend not to treat things in the latter categories the same, but all the same I would like to get to them eventually, so I'd prefer not to kick them out of my review queue. They're also frequently design tasks that are hard to delegate to people who aren't experts in the code.
Perhaps what would help the most is if anything that risks falling into the latter two categories had an explicit design review stage sometime before a 100K patch appears in somebody's review queue.
On Thu, Aug 11, 2011 at 2:34 PM, L. David Baron <dba...@dbaron.org> wrote:
> Perhaps what would help the most is if anything that risks falling > into the latter two categories had an explicit design review stage > sometime before a 100K patch appears in somebody's review queue.
IMO it's perfectly reasonable to bounce those sorts of reviews with "changes of this magnitude need some more discussion before I can review a patch against them; can you start a thread in .platform about it and put the link in here?"
> On Thursday 2011-08-11 10:01 -0700, Zack Weinberg wrote: >> I'm not sure that would help; it's been my experience (particularly >> in dusty corners of gecko core) that the module owners are the >> people who don't do their reviews.
> Yeah, that's probably me. I've been spending as much time as I can > on reviews lately (it's my primary job right now; I basically > haven't been writing code), but there's also a limit to the number > of hours a day I can spend doing code reviews and maintain my > sanity.
I actually wasn't thinking of either you or Boris; I trust both of you to come through with reviews eventually, and I know how swamped you are. (In fact, I have the impression that Boris is swamped partially _because_ he's the only person who will review patches in a timely fashion in large areas of the codebase.) You also usually tell people (or anyway, you usually tell me :) when you're not going to get to something soon.
My experience with other module owners has not been so good. To pick four examples not at random:
https://bugzilla.mozilla.org/show_bug.cgi?id=563192 Review requested from, AFAIK, the *only person* who can review changes to NSPR; no reaction whatsoever for over a year. (Have I mentioned recently that we need to get rid of NSPR?)
https://bugzilla.mozilla.org/show_bug.cgi?id=643041 Super-reviewer asked a third person for additional review; no reaction from that third person for five months. Super-reviewer also raised some fuzzy concerns; I asked some questions in response; no reply for five months.
https://bugzilla.mozilla.org/show_bug.cgi?id=305206 Not originally my patch, may fall into one of your problematic categories, definitely needs significant additional work - but without any feedback for over two years, I'm not motivated to do any of that work.
https://bugzilla.mozilla.org/show_bug.cgi?id=230675 Also not originally my patch, and may not even be a good idea, but illustrates the difficulty of finding anyone other than Boris who will admit to being qualified to review design-level changes to Necko.
> * [patches] from new contributors who need much more > careful review and mentoring
So I mostly don't have the authority to approve patches, but I *do* have the knowledge to mentor new contributors through getting all the rough edges off their first several patches. I would be happy to take on some of that effort, and I bet there are lots of other people who are also able and willing. Would it help to have a formal system for delegating that kind of patch review, or at least an informal (but written down) list of people who can do that?
> * patches that redesign a subsystem without previous discussion > with the other maintainers of the code (of, e.g., what the > constraints that led to the current design are or what other > changes need to happen soon in that area of code), in other > words, where the review request essentially requests either > rubber-stamping of the new design, or that a replacement design > be offered (which is a lot of work)
> * patches that fix the case that the patch author cares about but > break a lot of others -- in other words, patches where the author > has done the first 20% of the work, is asking me to do the next > 50%, and is quite willing to do the typing at the end to finish > the last 30%
I think that when a patch falls into one of these categories, reviewers should say so quickly and r-, but then offer a way forward for the author. In the first case, that could be something like "This redesign doesn't account for X, Y, Z constraints that may not have been obvious to you, and might interfere with A, B, C future plans in this area. Bugzilla is an awkward place to discuss redesigns; let's start a conversation on dev.platform (link to instructions for use of dev.platform)." In the second case, "This patch fixes this bug but introduces a whole bunch of other bugs -- please do the next 50% of the work" seems like a reasonable thing to ask.
People don't like being rejected, but they *really* don't like being ignored.
> Perhaps what would help the most is if anything that risks falling > into the latter two categories had an explicit design review stage > sometime before a 100K patch appears in somebody's review queue.
I've found it difficult to get any traction on bugs that ask for design (or review of design) ahead of there being any code to review - see for instance https://bugzilla.mozilla.org/show_bug.cgi?id=650966 which is a follow-up from an in-person discussion that was very productive, but despite that, has gone precisely nowhere.
Maybe that's also something we need new process for.
On Fri, Aug 12, 2011 at 9:41 AM, Zack Weinberg <za...@panix.com> wrote: > My experience with other module owners has not been so good. To pick four > examples not at random:
Thanks for turning the discussion to specifics!
> https://bugzilla.mozilla.org/show_bug.cgi?id=563192 > Review requested from, AFAIK, the *only person* who can review changes to > NSPR; no reaction whatsoever for over a year. (Have I mentioned recently > that we need to get rid of NSPR?)
Fork NSPR and make khuey own it!
> https://bugzilla.mozilla.org/show_bug.cgi?id=643041 > Super-reviewer asked a third person for additional review; no reaction from > that third person for five months. Super-reviewer also raised some fuzzy > concerns; I asked some questions in response; no reply for five months.
> Also not originally my patch, and may not even be a good idea, but > illustrates the difficulty of finding anyone other than Boris who will admit > to being qualified to review design-level changes to Necko.
That should change now that an actual Necko team exists.
If these reviewers were nagged, then they're being irresponsible (except maybe for NSPR which is just a black hole). (Not condemning those individuals; I'm guilty too.) I doubt we can fix that with process tweaks. It would be good to know the reviewer's side of the story.
> * [patches] from new contributors who need much more >> careful review and mentoring
> So I mostly don't have the authority to approve patches, but I *do* have > the knowledge to mentor new contributors through getting all the rough edges > off their first several patches. I would be happy to take on some of that > effort, and I bet there are lots of other people who are also able and > willing. Would it help to have a formal system for delegating that kind of > patch review, or at least an informal (but written down) list of people who > can do that?
I think it would be great if people like you could curate the review queue by looking for stale review requests, especially from people you don't recognize, figuring out what needs to be done (nag, polish, reassign, review it yourself, etc), and doing it?
> Perhaps what would help the most is if anything that risks falling >> into the latter two categories had an explicit design review stage >> sometime before a 100K patch appears in somebody's review queue.
There's no actual design in that bug. From the outside it's not clear to me what the next step in that bug was supposed to be.
I think a post to dev.something saying "I am working on a patch that does XYZ in the following way: ... does anyone have any feedback?" would be adequate. If there's no feedback, go ahead and do it; if someone pushes back on the design after that, you have the moral high ground!
"I want to do XYZ, can anyone tell me how it should be done?" is a little less likely to get feedback but also worth trying if needed.
Rob -- "If we claim to be without sin, we deceive ourselves and the truth is not in us. If we confess our sins, he is faithful and just and will forgive us our sins and purify us from all unrighteousness. If we claim we have not sinned, we make him out to be a liar and his word is not in us." [1 John 1:8-10]
> On Wed, 10 Aug 2011 14:33:18 -0700, Taras Glek wrote: >> Hi, >> I find that a lot of contributors(myself included) get discouraged due >> to having their patches rot in the review queue. >> It's not fair to the contributors have a patch rot without any feedback. >> It's also not great to r+ patch a month after it was submitted.
>> I propose a two step solution to this
>> 1) Every bugzilla account should have bugzilla >> whine(https://bugzilla.mozilla.org/editwhines.cgi or equivalent) turned >> on to remind reviewers about outstanding r? requests.
>> 2) Institute a policy where reviewers have to reassign r? to someone >> else if they know they wont be able to review something within a week. >> It might turn into a game of hot potato, or it might end up with r? >> flags being removed from attachments. I think that's better than quietly >> ignoring the problem.
>> I realize that some people do a lot more reviews than others, but >> letting patches languish in the queue doesn't make those queues smaller. >> We should have more fallback reviewers instead.
> How would you deal with people like, um, timeless, with review requests > going back to 2004.
I would declare bankruptcy and unset r? flags from those patches. There is no point in pretending that patches will get reviewed.
> On Thursday 2011-08-11 10:01 -0700, Zack Weinberg wrote: >> I'm not sure that would help; it's been my experience (particularly >> in dusty corners of gecko core) that the module owners are the >> people who don't do their reviews.
> Yeah, that's probably me. I've been spending as much time as I can > on reviews lately (it's my primary job right now; I basically > haven't been writing code), but there's also a limit to the number > of hours a day I can spend doing code reviews and maintain my > sanity.
So you probably have already thought about this, but is there one or more people working in your areas that you could see if they want to do reviews? Maybe just throw some small patches in their direction and see how they cope?
This isn't easy to apply everywhere, as we may not have people in appropriate areas, but it is something I'm thinking about very frequently.
> I was recently told of a company that escalates any expense report that > hasn't been approved in 24 hours to the next level of management up. It > might be helpful to something similar in bugzilla. If a review hasn't > been processed in say a week, bugzilla could automatically re-assign it > to the module owner (based on the product and component).
I don't think it should have to be re-assigned to the module owner, I think they should be the ones actively triaging/keeping an eye on the lists in their module.
