Growing reviewers

One of the challenges of a growing organization is that people become managers and have less time for coding. A scary proportion of module owners are managers now.

We were discussing this with Dietrich and he came up with a really simple solution: module owner’s entire team should be able to review patches for that module. Obviously new people can’t review every little detail, for those they can pass the buck to their manager(and learn in the process). I really like the new Firefox review policy of having a large set of candidates for reviews who have an option of passing the patch along.

Perhaps bugzilla can change r?:taras queries into r?:taras+his+team and do this automagically.

21 comments

  1. Or pay and give advantages to senior devs that are comparable to managers ;-)

  2. ohou, the problem is that we do not have methodology for adding new module owners. People don’t like extra responsibilities or feel they aren’t ready. This would let them do reviews on training wheels…and force them into it.

  3. Every JS member and peer can review patches. And there are some people not in that list who get asked to do reviews, because the technical policy differs from the desired reality. The former I thought was the actual policy, that more anointed people than just the module owner could review patches. Do other modules require owner signoff on every patch? That seems pretty ugh and bottleneck-y.

  4. How are you defining “team” here? Reviewers shouldn’t be chosen because they were hired by Mozilla.

    See http://groups.google.com/group/mozilla.dev.platform/msg/2393cfad02b346b0 for an alternative, and keep your hats straight. There are two sets, and they are different. I’ll object very strongly to any policy that gives someone who wears a MozCorp hat any advantage over someone who doesn’t have one. It should hold true that new hire == new contributor.

  5. @Jeff, no peers can do this too, but there are very few available peers.
    My examples are xpcom/editor/toolkit. I think shaver’s new Firefox policy with a busload of peers is the way forward.

  6. Also, passing a patch review “up” shouldn’t mean to your manager, it should mean to the module owner or peer. I have much more experience in my module than my manager, for example. It’d be absurd for me to escalate a review to him. (Again, keep your hats straight.)

  7. fantasai, I basically want what you are proposing. I’m suggesting using moco corp structure as a source for new reviewers. I actively encourage all qualified community contributors to my code to become peers. So far none of them have stuck around.

  8. Seems like the basic argument is “we should appoint peers more liberally”. I don’t think anyone really disagrees with that in general – there are some great benefits. But care needs to be given in how that’s done to ensure that code review quality doesn’t suffer too much.

    I think you should avoid referring to MoCo management structure when making this argument – it’s mostly irrelevant and can really rub people the wrong way.

  9. Taras: toolkit has plenty of active peers (see https://wiki.mozilla.org/Modules/Toolkit ). Where are you having trouble getting reviews?

  10. That does revive here some memories of Netscape. The size of Mozilla and its new strategic goals seem to make it align processes more with a corporate attitude than with a community-based attitude. That’s normal but all I can say is “been there, done that”. Long ago but still. There are still some people at MoCo who were there too.

    On the specifics of code review, since when A, member of team managed by B, has the review skills of B? That kind of assignment should be based only on pair-based recognition, not presence in a given team.

  11. What you and I are proposing are very different, if I go by what you wrote.

    You’re proposing that everyone hired into a team at MoCo be annointed as a reviewer in the module they work in. At their discretion, they may escalate a review up to their MoCo manager.

    I’m proposing that module owners and peers actively reassign initial reviews to qualified contributors (whether MoCo employees or not) and do a super-review (possibly rubber-stamped, if the reviewer knows that code well and is comfortable with the review) once the patch gets r+. Once such a junior reviewer seems to need no oversight across the module, nominate him/her as a module peer.

    Our module ownership and the review requirements were set up to be independent of employment status specifically to ensure code quality independent of management and hiring decisions. I think that’s worth keeping.

    My version doesn’t give any special consideration to status at MoCo. It cuts down on the current reviewers’ workload, but still ensures a qualified review. And it explicitly creates a mentoring environment for the new reviewers by requiring the second review: the new reviewer can learn from the super-review, and doesn’t have to worry about whether they’re fully qualified before giving it a shot.

    We’re roughly doing what I propose already; I’m just suggesting we be more aggressive about this kind of review re-assignment, and formalize the owner/peer’s super-review so everyone’s comfortable with the increased aggression. We could even encourage contributors to proactively take on patches in areas they’re qualified and interested in, knowing that either way the patch is guaranteed a quality review before it goes in.

  12. Fantasai, I like your proposal.

    It would be nice to have some lists of people
    who are willing and capable doing reviews.
    I mean developers who aren’t necessarily peers or
    module owners. Such lists could be changed easily and often,
    and could be hopefully even hooked up to bugzilla, so that
    when someone has asked a review from me, I could just
    check who else is available doing reviews in the module and
    re-assign the review request.
    Then, once that reviewer gives r+, I could read the code.
    This does mean bringing back superreviews for many cases, but
    hopefully those superreviews would be light weight.

    And yes, MoCo teams don’t have much to do with reviewing.

  13. I also like Fantasai’s idea, I am not a developper myself despite having a couple of simple patches in toolkit and firefox, I asked reviews in the past to people that are visible as reviewers just because I had no idea who to ask and I felt bad pinging people with a lot of work on their hands for such simple patches while I know now a few non-moco hackers that could have done the review but were listed nowhere. If this list were available somewhere, I wouldn’t have to worry again of putting even more work on key developpers by asking them reviews for my micro-patches.

  14. FWIW, we actively hand out reviews in MailNews/Thunderbird. If someone’s written a substantial chunk of code, they’re generally the best person to review smallish fixes to bugs in it. (I was reviewing small enough patches long before I became a peer.)

  15. “Our module ownership and the review requirements were set up to be independent of employment status specifically to ensure code quality independent of management and hiring decisions. I think that’s worth keeping.”

    “That does revive here some memories of Netscape. The size of Mozilla and its new strategic goals seem to make it align processes more with a corporate attitude than with a community-based attitude.”

    That was a different era and a completely different circumstance. Netscape and its management did not exist for the sole purpose of advancing the Mozilla mission. They were conflicted. That was the big problem with Netscape. Mozilla and its management do exist for the sole purpose of advancing the Mozilla mission. Mozilla is not conflicted.

    I’m all for making sure we don’t miss volunteers or put artificial barriers up for volunteers, but Mozilla is not Netscape and many of the policies and processes we built back then (I was a part of the team making those policies) were defensive policies designed to keep Netscape from f**king over the Mozilla project.

    If, as it sounds, you and Daniel believe we need policies to keep Mozilla from f**cking over the Mozilla project, that somehow Mozilla’s companies have become hostile to the Mozilla project then I think your focus is misplaced and you should instead be calling for the Foundation board to replace that supposed hostile management.

    I’ll say finally, that Module Owners are free to set policy for their modules an if they want to build delegation systems that have assumptions about the capabilities of the people they hire onto their team, I believe our rules allow them to do that.

  16. fantasai, I see your point. I should’ve phrased it better. I don’t think everybody appointed into mozilla should become a peer because they managed to get hired. I do think they should have to review patches with aim to eventually become a peer.

    I think your proposed system is good. My main point is that having lists of potential reviewers is not enough. Under the current system those would be far smaller than required. My blog post is a suggestion on how we can continuously fill/refresh those lists.

    Gavin, I agree that discussing how moco structure ties into a project is tricky. However the fact remains that moco is the single biggest source of contributors and we are not taking advantage of it sufficiently. I don’t see how having a moco review bottleneck benefits the community in any way.

  17. I don’t know where you get “MoCo review bottleneck” from. MoCo has never had anything to do with code review policies, and I feel pretty strongly that that should continue to be the case. There are reviewers inside and outside of MoCo, and there are bottlenecks inside and outside. There’s no benefit to tying your proposed solution to MoCo’s management structure, so why do it?

  18. Asa, MoCo is not Netscape, but the more employment appears to confer special privilege the more discouraging it is to community members and especially potential new members. That will hurt us.

  19. Asa, nobody is accusing MoCo of acting in bad faith. However, IMHO designing a system under the assumptions that managers are more qualified reviewers than their reports; that MoCo hires are qualified reviewers regardless of actual experience; and that non-MoCo engineers are second-class contributors (less qualified / less willing to contribute) is dumb-stupid because none of these assumptions hold true. See also dveditz’s comment.

  20. I’m curious why the community-members-turned-into-peers (as mentioned in the seventh comment) have not stuck around; what could change such that they do? Do they just lose interest in Mozilla? If yes, why?

    I expect the answers to that to be varied and complicated, but important. But you probably don’t have them either ;)

  21. Mook, I’m sure we don’t have all of the answers, but I would expect a lot of it is just that community members are often people not working on Mozilla on a day to day basis. They are giving their spare time up to us and no-one has an unlimited amount of spare time.

    I think what Taras is getting at is that while we should certainly be growing reviewers both from the community and the paid staff the means to do so can differ a little. With a community member I can certainly ask them to do reviews to help build them into peers. Whether they do them or not depends on their available spare time and dedication. With staff however they are paid to work on Mozilla so it is more likely that the reviews will get done and so they will learn and become peers.

    My experience has been that the first steps a staff developer makes in Mozilla are used to help them gain commit access, I’ve never particularly seen a similar effort to help them become reviewers. It always seems a little more haphazard so I’m sure there are better things we can do there.