You may recall we are doing some work to try and clear the backlog of old requests (review, super-review, feedback etc.) This first involved sticking a CSV template on bugzilla.mozilla.org so we could monitor the number and age of outstanding requests. Since mid-December, I’ve been downloading a full dump of the outstanding reviews each day. Then, in mid-January, we implemented a gently nagging email, to be sent every week, reminding people about their requests older then 7 days. I have now written a script to analyse that data, to see if the email has had any effect.
Here’s what happened, looking at the most common type of request (“review”), in terms of the number of review-days outstanding[0]:
The first noticeable downtick in the graph is just after the first nagging email got sent out, and as you can see, the effect continued weekly for about a month, before vanishing, and the level plateaued again. I would interpret that as an indication that all the low-hanging fruit is now gone. And there’s still a lot of outstanding reviews – 928 older than 90 days (down from 1192).
We reduced the number of outstanding review-days. But was this due to people dealing with some old ones, or were people getting more timely with newer ones? If we do the graph again, this time ignoring all reviews older then 30 days, we can see that (if we assume the number of new review requests per day is roughly constant, which as far as I can tell it is) there’s not been much reduction in waiting time:
So what do we do now?
I think we want to get to a place where no request is outstanding more than 30 days, and where someone with contributor engagement can monitor requests as they cross that threshold and try and see why and when something went wrong. On average, 4.4 reviews a day cross the 30 -> 31 day threshold. As reviews are the most common type of request, I’d suspect it’s about 5 requests a day.
Here are some options to clarify the situation and make it better in the future (of which we could do more than one). Note that here I am talking about requests which have the ability to take an explicit requestee (such as “review”).
- Ban new requests “of the wind” (ones without a named requestee)
- Ban new requests of requestees who have not been active in Bugzilla for > N days (suggested N: 30)
- Cancel existing requests of requestees who have not been active in Bugzilla for > N days (suggested N: 30)
- Cancel existing requests on VERIFIED bugs (are there any which could validly be on such bugs?)
- Cancel existing requests on RESOLVED bugs (are there any which could validly be on such bugs?)
- Cancel existing requests which have been outstanding > N days (suggested N: 30)
- Find someone to examine requests which cross an N-day boundary and investigate them (suggested N: 30)
All bans would be accompanied by advice on finding an appropriate reviewer. All cancellations would be accompanied by an apologetic comment and useful advice for the patch submitter about steps to take next.
Thoughts?
[0] Note: I’m no statistician. I’m happy to give my collected data to anyone else who wants to do some analysis. Just email me. Anyone who wants a CSV dump of all current requests, it’s here.
1New comers might not know how to ask reviews to, hence the to the win ones. How about banning the fact that you can make them and gently propose default reviewers based on the component instead of an empty field.
2 how do you deal with 3 on smaller components (I’m thinking NSS here even if things have changed in the last few months)
4 and 5 how do you ensure the requests aren’t for say adding unit tests ?
N30 is very low for some omponents.
We should also try to spread the reviews among reviewers more evenly.
That could, hopefully, reduce the overall waiting time to get a review for a patch.
Perhaps bugzilla could hint that in module X common reviewers are
foo, bar, baz, and of them foo has already reviewed tons of patches lately and
has still long review queue so it might be better to ask bar or baz to review the patch.
Nice Idea !!
If we are to propose default reviewers, then there needs to be some computer-readable database of reviewers somewhere. We don’t have that at the moment. We may just have to ask people to read a list.
N=30 may be wrong for the inactivity criteria; perhaps N=60 would be better? If someone hasn’t logged into Bugzilla for 2 months, it’s unlikely they are going to review your patch…
Good question about unit tests; we may have to restrict that to certain review types, or put an age on them as well.
I think the banning steps and the examination one are worthwhile, but I’m not so happy with anything that automatically cancels any requests. That’s not solving the problem, it’s just hiding it. The unreviewed patches would still sit around, but now even without a review flag and so nobody would ever notice them. Concentrated efforts to triage those bugs is good, automated canceling can always remove actually important stuff or just hides problems instead of solving them and should be avoided.
“If someone hasn’t logged into Bugzilla for 2 months, it’s unlikely they are going to review your patch…”
Well yes, but in order to have a positive effect, the patch author or someone else then has to pick that up and make a new request. Patches sitting without review requests is already a problem, so I guess this would just make that worse. What needs to happen in (most of? at least some of) these cases is that the review request gets re-directed to someone who will do the review. Just cancelling doesn’t seem to achieve that.
I’m afraid that the “cancel” proposals may need to be changed to “find someone to review”…
Sadly I have seen more than one discussion on IRC about people choosing to filter the request reminder bugmails directly to trash :-(
That’s sad. There’s someone on the other end of every request, waiting for you to do something. The least you can do is tell them that you aren’t going to do it (by cancelling the review). That hardly takes long…
The idea would be that if a request was cancelled, a comment would be added explaining to the reporter how to follow-up. Perhaps the right thing to do is just to check the patch and re-request review (in which case, the review would be a “new” one, and handled using the new system), or it might be to abandon it, or something else.
The point is, what is happening now is that the requestee is being misled into thinking the requester is going to do something. That’s not true; the requestee finding out it’s not true can only be a positive step.
Unfortunately, we don’t have a good programmatic way of allocating reviews to particular people – and if we wait to develop one, we’ll never get anything done about the problem. Asking the patch author to do some work manually isn’t awesome, but it’s much more likely to produce a better outcome quicker.
I’m one of them. I find it very irritating to get this weekly notification. I know what sits in my review queue, and I also know which ones are blocked by other bugs before being reviewable. And there are those requests which are rather WONTFIX or P5++ and are ultra-low priority compared to much more important tasks. And there are those requests where we ask for more information from the patch author, and never get it, delaying the review by several weeks.
Write an extension. If the requestee field is empty for flags named “review”, set the requestee to some given reviewer based on the product + component. The list would be hardcoded in the extension or in a separate CSV or XML file so that you have control on who accepts to be a default reviewer based on his free time and motivation.
Cancelling a review request is the best way to put the patch out of reviewers’ radar. Definitely not the right thing to do. I don’t see how leaving a patch with a review request on it is a problem. A problem for whom? The reviewer or the patch author? The patch author can comment in the bug about review progress, and someone in the CC list will probably reply. If there is nobody in the CC list, nor watching the bug, then it’s probably very-low priority or even an unwanted feature or bug fix. No need to take (automatic) actions for these bugs.
So, whaty do you propose that Bugzilla should do the next time I typo the reviewer’s email address?
The same thing – tell you there’s no account with that email address. Why do you think I’m proposing anything different? :-)
Thanks Gerv for this, it’s really important. It’s frustrating to see train after train leave while a valuable patch just bitrots waiting for a review.
I’m curious, how does the reminder mail know when a reviewer has responded? Is it only when they give it +/- or is adding a comment to the bug enough? For example, I put one patch up for review nearly 4 months ago and the reviewer soon commented, “Hmm, I don’t know”. I’d like to think they’re getting nag mails from more than just me. :)
Also, I’m not sure about measures 4 or 5. For security bugs we’ll land the test patch separately after the fix has shipped and it’s possible the fix could get landed and even verified while there are still pending reviews on the test patch (though typically the test patch gets reviewed very quickly).
I think a deeper issue is knowledge sharing. There are so many patches where only the same 1 or 2 people are qualified to review it (and hence they’re swamped). That’s a different discussion though and this is still a useful step.
We have attempted to generate such a list in the past, but it’s not simple. Bugzilla is large. And having it centralized means it’s much more likely to go out of date.
They need to fulfil the review – cancel it, + it, – it or reassign it. So yes, your 4-month-old review is included in the nagging.