24-hour reviews

I would like to see Firefox developers switch to 24hour review turn-around times. Note that in my definition review turn-around means any of the following:

  • r+/r-
  • unset/reassign r? to someone else

It is ridiculous in our recent faster release cycle if a patch takes half (or more) of the cycle loitering in the review queue. I believe that a shorter review cycle is the simplest way to accelerate Firefox evolution.

I view fast review times as a matter of respect. Posting a patch usually requires a significant time/effort commitment, reviewers should act appropriately. There is no bigger buzzkill than having your work pushed back to the bottom of somebody’s TODO list like some annoying chore.

As far as I can tell there are 3 main reasons* that lead to long review times:

1) People like gavin, bz, dbaron having disproportionally high review loads. We need a process to hand-off patches to other reviewers. High-load people shouldn’t shy away from passing on the r? to someone else when possible.
2) Bugzilla-fobic people (like myself) loosing track of bugzilla r? requests due to not having bugzilla whines setup. Bugzilla whines should be enabled by default.
3) Bad review habits.  I met a number of Mozilla developers that like to batch their reviews up and then do them all on a single weekday. Please stop, you are killing all kinds of coding momentum/fun/etc.Lets make it our policy to set aside time every day to clear the review queue.

Clearly people with existing backlogs will take a while to catch up, but most MoCo employees should be capable of this.
I have yet to hear a good reason against doing daily reviews.

It has been a few months since I proposed this on dev.platform.  I have tried to live by the 24hour rule, I think a few others tried this too. I find that morning bugzilla r? whines work best for me. I still occasionally loose track of a patch for a few days, but nobody is perfect. I think people appreciate fast reviews, but nobody thanked me yet.

Dec 6 Update: My goal is to have *some* response within 24hours with an ETA for next followup in the worst case.

22 comments

  1. You can also set your review queue as a pinned tab if that helps.

  2. Sad to say, for over half of all the patches I’ve ever done, one of bz, dbaron, roc, or smaug was the _only_ qualified reviewer.

  3. I think it’s a worthy goal but as long as absolutely anyone can submit review requests and so much of the product still only really has one or two people qualified to review the code it frequently becomes a trade-off of reviewing a patch for a small issue that a contributor wants and completing a feature that the product drivers are chomping at the bit for. Until we figure out how to better ramp up more developers for much of the product we are probably stuck with this problem.

    I also don’t think just r+/r-/reassign are enough options. I frequently want to ask for clarification about patches before I r+/r- and that can take days to get turnaround from patch authors.

  4. @Zack perhaps you can help them review now.

    @Dave. true. I meant that there has to be some response within 24hours.

  5. My previous comment was too telegraphic (I blame Android). There should be several more people in the list of “only qualified reviewer”s; bsmedberg, for instance. But _who_ is not the point. The point is, it appears to me that for a majority of the Gecko code exclusive of the JS engine (which I have hardly ever touched), there are no more than two people who are qualified to do reviews _at all_. That these people are so overloaded that they cannot do reviews _quickly_ is merely a symptom of the problem.

  6. Zack makes an important point – many times there’s not more than 1 qualified reviewer for a given area of code.

    I think that a primary responsibility of overloaded reviewers should be to *grow new reviewers*.

    It’s not too hard:

    1. receive a review request
    2. re-assign it to someone who you think could be a good reviewer if they knew the code.
    3. repeat above until the person is good enough to be a reviewer, and then knight them.

  7. “if they knew the code” isn’t quite enough here.

    Really, I think reviews should be separable into two pieces. Given a patch and a description of what the patch does (often not provided), there are two things to review:

    1. is the description of the patch the thing we want to do

    2. is the patch the right way to implement that described behavior (correct, with appropriate performance, memory use, and maintainability)

    The reviews I do vary quite a bit as to whether reviewing (1) or (2) is more work.

  8. Yep, if there aren’t enough reviewers to go around, making the review process _slower_ (i.e. what currently happens) isn’t going to help getting more people to become reviewers.

    There’s also things the requester can do, too – feedback? exists, if the patch isn’t going to quite make it. (I used it recently; the response came within a week, which I figured was decent given past project-wide r? turnaround times.)

    Everybody knows about request.cgi, right? :)

  9. can someone help me understand how to set up a query for my review queue? :( I feel soo dumb

  10. I agree with the will to get fast reviews, but I don’t think the 24 hours is a realistic time for every case. Even just because the review request may come in the middle of work on another large review, or while you are changing some complex code, or you are just not around.
    In some cases I also want to take a bit of time to think about the approach and the possibility for better approaches, and a global idea of how the patch inserts in the surrounding code. Having to rush a review would often bring to low quality approaches. Plus, as Dave said, one may need clarifications.

    But surely, “not more than a week” would be an achievable target, leaving the reviewer time to examine the approach, and not being too annoying for the developer.
    Obviously it clearly depends on the patch and what it touches, the other day a contributor posted a patch fixing a js warning, asking who could review it. I saw the bugmail and he got a review in about 1 minute. But it was a trivial patch, I posted a 400KB patch the last month and I would have never expected to get a review in 24 hours!

    Looking at the issues you posted:

    1) We have a feedback? flag, and it was also created to allow people like gavin, bz, dbaron forwarding a first-review request to others. This also allows to grow new reviewers.

    2) Having a livemark showing all of your review requests is trivial. It’s practically impossible to lose a request, I have one in my bookmarks toolbar and I check it multiple times a day.

    3) Sometimes, you don’t make reviews in a weekday because it’s nice to do so, but because you are coding some hard path. All of our reviewers are also awesome coders, and often they work on the hardest parts of the codebase. Switching context to reviews in certain cases really hurts your work. Doesn’t happen often, but it can happen. I remember when Sdwilsh tried to pursue the effort to review in 24 hours, the result was he found difficulties in recollecting time and concentration to code.

    So, reading your blog post my impression is that reviewers are lazy guys not willing to solve a so easy issue, but reality is not so easy.

  11. This is the query for my requests queue, you can easily make your query replacing the bugmail.
    You can add the bugzilla query as a livemark to your bookmarks toolbar, or just keep it in an app-tab.

    https://bugzilla.mozilla.org/buglist.cgi?field0-0-0=requestees.login_name&type0-0-0=equals&value0-0-0=mak77%40bonardo.net

  12. @gandalf Just go to “My Requests” in the list of links at the top of the page in bugzilla. That will show you everything you’ve requested and everything requested of you – but you can then filter it down to what you want.

    You also get when the reviews were requested so you can see how old they are.

  13. In my experience, overloaded reviewers are usually a symptom of a system that has grown overly complex or large and needs to be properly split into components.
    For a start, this leads to two or more components that are smaller. But it also leads to a (hopefully well-)designed and documented interface between these new components that is easy(er) to understand and allows one to get competent in the codebase quickly.

    Unfortunately, such a task is usually itself complex and can only be done by the maintainers themselves, so it will slow down a slow part even more. And designing an interface isn’t easy either, because it involves untangling a lot of things without introducing lots of bugs and performance issues.

    I should also point out that “just grow more reviewers” at this point is usually a problem, because even a competent developer will need a lot of time to learn the ins and outs of a complex and large codebase.

  14. The engpm team is doing some work in this area.

    1. Check out the bugzilla anthropology wiki, which details a larger project of understanding our current process and how we all work with bugzilla
    https://wiki.mozilla.org/Bugzilla_Anthropology

    2. mbest has started working on a review dashboard. This is very early work and he is open to input as to what would be useful in a dashboard. Some suggestions have included review queues, average response times, identifying review requests from new vs existing contributors, and identifying large queues waiting on specific reviewers.

  15. I have noticed that the smaller the patch the faster the review (gross overstatement, but trendable). I personally try to do my reviews within 3 days, usually the same day. There are some which I don’t know about and have to do some research (either code, api’s, testing, asking on irc).

    Maybe we should have a reviewer stat system:
    * average time to response to a r? or f?
    * average size of reviews
    * average # of reviews / day

    Doing a r- should be acceptable if:
    * there is not enough information in the patch or description
    * you are not fully qualified to do the review
    * you don’t have time to review it within 3 business days (but you need to give it to somebody else)
    * patch sucks
    * patch is too long and could reasonably be broken up into smaller patches.

    just some thoughts.

  16. On the mobile front-end team, we’ve worked hard to make sure that nearly all of our code has far more than one or two qualified reviewers. One way we do this is training new contributors to become reviewers very quickly.

    Once a new hire has had a couple of their patches reviewed, I try to start sending them review requests of trivial patches, followed soon by more-involved patches in code they’ve worked on themselves, followed by patches in areas that they haven’t even seen yet. The latter may take more time and more back-and-forth because the new reviewer has to do more research to figure out the surrounding code, but it’s a good way to learn the code base and to become confident as a reviewer. If there are decisions the new reviewer isn’t yet qualified to make, they or I can request a second review from a more-experienced peer.

  17. @mbrubeck:

    I think the mobile front end has an advantage here, in there’s not 12 (or more!) year old legacy code that has to be understood.

    @jmaher:

    The important part is that time to review doesn’t scale linearly with patch size, it scales quadratically (or worse). A bunch of bite sized patches are much easier to review than one monolithic patch.

    I applaud the sentiment here though. 24 hours is a bit unreasonable (there are weekend and such, and not everyone who would be reviewing is paid to work on the project) but you’re right that burning most of a release cycle in a review queue is a situation to be avoided.

  18. Matt Brubeck: I really appreciate the work you’ve been doing for review turnaround in mobile. It’s the best Firefox team I’ve worked on for reviews.

  19. A couple of reiterations of things that have already been said, but are worth saying again:

    1. *Some* response within 24 hours. I think that’s a fantastic goal, and best of all, it’s a measurable goal.

    2. The feedback? flag should be used more than it is, and perhaps more often than the review? flag. Those should also be answered within 24 hours, though.

    3. Not enough reviewers? Grow more reviewers.

    4. Flipping the review? flag to someone can be as impolite and shocking as tapping someone who wasn’t paying attention during a meeting on the shoulder and saying “What do you think?” Developers submitting a review should take the time to spell out what the patch does, if there are specific areas that they know bear close review, and what dead paths they’ve tried before. Show your work.

  20. How about breaking the problem into approachable steps? Getting 24-hourish turnaround on large patches to weakly-owned code is going to be a Hard Problem To Solve. But I think there’s a _lot_ we could do for other reviews.

    – There should clearly be _fast_ reviews on tiny and simple patches. But they’re all the same to Bugzilla and bugmail. Having a way to prioritize these would be fantastic. Some trivial patches could then review within minutes.

    – Semi-related, patches that go through multiple rounds of review suffer. Even large patches are effectively “small” as they near acceptance, but again Bugzilla doesn’t indicate this (beyond the attachment description, which may or may be useful). The interdiff tools also fail to help much due to brokenness. :(

    Just these two would help a lot, and are a bit of a positive-feedback loop… Faster turnaround for small patches = encouraging breaking up large changes into small steps = easier review = faster turnaround.

    (That said, I don’t want to imply like I’m blaming everything on Bugzilla. Just that there might be some cheap wins for some parts of the overall issue.)

  21. I broadly agree with Taras, and I think everyone’s taking the wrong approach by focusing on all the edge cases where it might break down. Consider the 24-hour window as a rule of thumb; of course there will be exceptions, but it’s still a worthy goal.

    Growing new reviewers is also important. I’ve written some patches lately for the storage module, for which only two people can do reviews (the module owner and its single peer), both of whom are now very part-time Mozilla contributors.

  22. There’s an easy quicksearch shortcut:

    https://bugzil.la/review?name

    “feedback?” also works.