Relevance of Super-Review (Was: Hardening the review requirements for changing .webidl files)

[Bobby Holley]

(I want to avoid entangling the dom/webidl plan with this discussion, which
is why I forked the thread)

On Thu, Apr 24, 2014 at 3:22 PM, Ehsan Akhgari <ehsan....@gmail.com>wrote:

> Following up on this, people asked us to not abuse the superreview flag for
> this purpose


If this is "abuse", doesn't that demonstrate that the super-review policy
is pretty much irrelevant to the modern world, and should be changed or
removed?

This rule seems like a textbook use for the sr? flag, aside from the fact
that the reviewers must be DOM peers and not people from [1]. But that list
is pretty out of date - there are several people who haven't touched Gecko
in over 3 years, and our WebAPI tech lead isn't on the list.

It seems like we should either update the list, or remove it.

bholley

[1] http://www.mozilla.org/hacking/reviewers.html

[Gavin Sharp]

Those asides are precisely the reason it's "abuse" :)

We should update the list, but from a quick skim I think there aren't
more than 2-3 names on that list that need removing. Part of the
problem might be solved by introducing an "superreviewer emeriti"
list.

Gavin

> _______________________________________________
> dev-b2g mailing list
> dev...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-b2g
>

[Ehsan Akhgari]

On 2014-04-24, 6:36 PM, Bobby Holley wrote:
> (I want to avoid entangling the dom/webidl plan with this discussion,
> which is why I forked the thread)
>
> On Thu, Apr 24, 2014 at 3:22 PM, Ehsan Akhgari <ehsan....@gmail.com
> <mailto:ehsan....@gmail.com>> wrote:
>
> Following up on this, people asked us to not abuse the superreview
> flag for
> this purpose
>
>
> If this is "abuse", doesn't that demonstrate that the super-review
> policy is pretty much irrelevant to the modern world, and should be
> changed or removed?

In my experience, different people have different ideas of what
super-review means. Some just ignore it completely, some adhere to the
definition that we have documented (an architecture level review) and
some seem to just ask superreview? as a higher level review of sorts
(they're probably confused by the name of the flag.) Going through our
hg log a few months back, a good number of sr=<nick> flags in the commit
messages are actually from people on the list of super-reviewers, and
there are some examples where other names appear there too.

Also, this topic seems to come up every few years. :-)

> This rule seems like a textbook use for the sr? flag, aside from the
> fact that the reviewers must be DOM peers and not people from [1]. But
> that list is pretty out of date - there are several people who haven't
> touched Gecko in over 3 years, and our WebAPI tech lead isn't on the list.

FWIW I'm not a DOM peer either. :P

> It seems like we should either update the list, or remove it.

I think there is still some value in the idea of an architecture level
review at least for people who are aware of our review policy, but I
agree that updating the list probably makes sense. IIRC the last times
when this was brought up people agreed that we should update the list,
but nobody ended up doing the work.

Cheers,
Ehsan

[Bobby Holley]

On Thu, Apr 24, 2014 at 3:58 PM, Gavin Sharp <ga...@gavinsharp.com> wrote:

> Those asides are precisely the reason it's "abuse" :)
>
> We should update the list

What is the list good for, exactly? There doesn't seem to be any consistent
usage of it anymore. In the areas that I work on (JS, XPConnect, DOM, and
other internals), it's occasionally used to indicate that the reviewer is
being requested for a more high-level review. But whether the requestee is
on that list never seems all that relevant.

Put another way - without a clear rule, requesting sr is basically a
judgement call these days. Given that, curating a strict list of potential
requestees doesn't buy us very much.

bholley

[Gavin Sharp]

(moving dev-b2g to bcc because cross-group threads are evil)

We do have fairly clear rules: http://www.mozilla.org/hacking/reviewers.html

(The definitions of "Significant" and "API" are somewhat subjective, though
it's impossible to come up with completely objective definitions - IIRC
there were long newsgroup threads about this when this version of the
policy was created. Maybe more guidelines or concrete examples as part of
the policy would be useful.)

I agree with you that that policy isn't being followed our enforced
consistently across the project currently. "Every patch must have SR" was
an easy policy to enforce, this one is much trickier.

I don't frankly know how I feel about "the value of SR" today. I'm curious
what others think.

Gavin

[Chris Peterson]

As we consider the value of super-reviews, we should include the
relationship between module owners and peers.

Long review queues are a burden for reviewers. Slow review turnarounds
force patch authors to juggle multiple patches to stay productive, but
this has a high context-switch overhead. Technical fixes are in the
works, such as new review tools and patch lint scripts, but we should
consider non-technical fixes, too.

Can we increase the number of reviewers by mentoring more developers to
become module peers, especially non-staff contributors? Review workload
would be balanced over more peers. We give more responsibility and
mentorship to developers, increasing knowledge and feeling of ownership.
This decreases the "[hit by a] bus factor" and grooms the next
generation of Mozillians. :)

Admittedly, increasing the number of module peers might require relaxing
the requirements for peers' module experience. For comparison, Facebook
is said to allow any developer to review any patch for any product.
However, Facebook's motto is "move fast and break things" and I am NOT
suggesting or endorsing that attitude for Mozilla. :)


chris

[Doug Turner]

If my git fu is correct, we only landed 180 patches with sr=. In 2009, we landed 1033 patches with super review. Of the sr= that landed in the last year, most were sr’ed by people from the DOM team (olli, sicking, blake, bz, sicking, sicking, sicking).

I tend to think that super review is a dumb idea.


I don’t think we need any new module level system for dealing with .webidl changes. A DOM peer is required. This group can impose whatever they need to keep code quality and API consistent high.


--
Doug Turner


On Thursday, April 24, 2014 at 4:31 PM, Bobby Holley wrote:

> On Thu, Apr 24, 2014 at 3:58 PM, Gavin Sharp <ga...@gavinsharp.com (mailto:ga...@gavinsharp.com)> wrote:
>
> > Those asides are precisely the reason it's "abuse" :)
> >
> > We should update the list
>
>
> What is the list good for, exactly? There doesn't seem to be any consistent
> usage of it anymore. In the areas that I work on (JS, XPConnect, DOM, and
> other internals), it's occasionally used to indicate that the reviewer is
> being requested for a more high-level review. But whether the requestee is
> on that list never seems all that relevant.
>
> Put another way - without a clear rule, requesting sr is basically a
> judgement call these days. Given that, curating a strict list of potential
> requestees doesn't buy us very much.
>
> bholley

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org (mailto:dev-pl...@lists.mozilla.org)
> https://lists.mozilla.org/listinfo/dev-platform
>
>

[Boris Zbarsky]

On 4/25/14, 10:06 PM, Doug Turner wrote:
> I tend to think that super review is a dumb idea.

It's a hack around people doing bad reviews is what it is.

There are certainly patches that are landing without official sr but
that do the moral equivalent of what sr is supposed to ensure: running
the API design by someone other than the reviewer whose judgment the
reviewer trusts (via feedback or needinfo or whatnot).

As long as all our reviewers are competent enough to know when to do
this (aka "know what you don't know") we don't need an official sr
requirement. In practice we seem to have been dealing without official
sr, as you note.

Of course if all our reviewers knew what they don't know we also
wouldn't need a commit hook on dom/webidl to catch patches there landing
without DOM peer review.... ;)

-Boris

[Mike de Boer]

Another rather nice purpose for the s-r flag, in my experience, is exposed during onboarding of fresh, remote, engineers (like I was ~1 year ago): it allowed me to get acquainted with the review cycle while working on Good Next Bugs(tm) that touched intricate parts of the codebase, like DocShell, and the knowledgeable colleagues behind them.
Also the social part of that process, I mean.

Being remote makes one (me) rely much more on the process facilitated by the tools in use and there are people behind them. And history... lots of that. In other words, this particular part of the process, s-r, made me connect with people that I respect and now know a bit better.

Whether the s-r flag is meant for this purpose or not is debatable, but this is just what I experienced in practice. Do with this info as you please ;)

Cheers,

Mike.

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org

> https://lists.mozilla.org/listinfo/dev-platform