Is super-review still a thing?

[Kris Maglione]

I can't remember the last time I saw a super-review request, but
it's still documented as a policy[1]. Is it still a thing? Do we
want it to still be a thing?

The reason that I ask is that I have a problem that I think I
might be able to solve by co-opting the super-review flag, but I
don't want to trample on it if we're still using it as
originally designed.

My problem is essentially that, for a few areas of code, I'm the
final arbiter, or at least the main blocker, for a lot of large
or critical architectural or API changes. The upshot of that is
that I get a lot of review requests for patches in those areas,
and most of the patches that make it into my queue are large
and/or complicated. This situation gets exhausting after a
while. And since people know that I tend to be busy, they also
avoid flagging me to review smaller patches, even when I really
*should* look at those patches (and therefore notice issues with
them only when I read code later).

For a lot of these patches, my opinion is only really critical
for certain architectural aspects, or implementation aspects at
a few critical points. There are other reviewers who are
perfectly qualified to do a more detailed review of the
specifics of the patch, and have more spare cycles to devote to
it. Essentially, what's needed from me in these cases is a
super-review, which I can do fairly easily, but instead I become
a bottleneck for the code review as well.

So, for the areas where I have this responsibility, I'd like to
institute a policy that certain types of changes need a final
super-review from me, but should get a detailed code review from
another qualified reviewer when that makes sense.


Does anyone have any objection to this plan? Or any suggestions
for another way to go about it?

-Kris

[1]: https://www.mozilla.org/en-US/about/governance/policies/reviewers/

[L. David Baron]

On Friday 2018-04-20 14:23 -0700, Kris Maglione wrote:
> For a lot of these patches, my opinion is only really critical for certain
> architectural aspects, or implementation aspects at a few critical points.
> There are other reviewers who are perfectly qualified to do a more detailed
> review of the specifics of the patch, and have more spare cycles to devote
> to it. Essentially, what's needed from me in these cases is a super-review,
> which I can do fairly easily, but instead I become a bottleneck for the code
> review as well.
>
> So, for the areas where I have this responsibility, I'd like to institute a
> policy that certain types of changes need a final super-review from me, but
> should get a detailed code review from another qualified reviewer when that
> makes sense.

I think it's reasonable to use the super-review flag for this sort
of high-level or design review, at least until we come up with a
better name for it (and make a new flag, and retire the old one). I
don't think the super-review policy (as written) is meaningful
today.

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla https://www.mozilla.org/ 𝄂
Before I built a wall I'd ask to know
What I was walling in or walling out,
And to whom I was like to give offense.
- Robert Frost, Mending Wall (1914)

[Gregory Szorc]

FWIW I'm pretty sure Phabricator won't support the super-review flag. And
since we're aiming to transition all reviews to Phabricator...

[Emma Humphries]

I’m away from my computer until the morning, but I think we disabled the super-review flag.

If Kris and David want to draft an architectural review policy that would be useful, and we could set up the flags at the right level

> On Apr 20, 2018, at 23:51, L. David Baron <dba...@dbaron.org> wrote:
>
>> On Friday 2018-04-20 14:23 -0700, Kris Maglione wrote:
>> For a lot of these patches, my opinion is only really critical for certain
>> architectural aspects, or implementation aspects at a few critical points.
>> There are other reviewers who are perfectly qualified to do a more detailed
>> review of the specifics of the patch, and have more spare cycles to devote
>> to it. Essentially, what's needed from me in these cases is a super-review,
>> which I can do fairly easily, but instead I become a bottleneck for the code
>> review as well.
>>
>> So, for the areas where I have this responsibility, I'd like to institute a
>> policy that certain types of changes need a final super-review from me, but
>> should get a detailed code review from another qualified reviewer when that
>> makes sense.
>
> I think it's reasonable to use the super-review flag for this sort
> of high-level or design review, at least until we come up with a
> better name for it (and make a new flag, and retire the old one). I
> don't think the super-review policy (as written) is meaningful
> today.
>

> -David
>
> --
> 𝄞 L. David Baron http://dbaron.org/ 𝄂
> 𝄢 Mozilla https://www.mozilla.org/ 𝄂
> Before I built a wall I'd ask to know
> What I was walling in or walling out,
> And to whom I was like to give offense.
> - Robert Frost, Mending Wall (1914)

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

[Tom Ritter]

Does it support the feedback flag?

On Fri, Apr 20, 2018, 5:03 PM Gregory Szorc <g...@mozilla.com> wrote:

> FWIW I'm pretty sure Phabricator won't support the super-review flag. And
> since we're aiming to transition all reviews to Phabricator...

[Dave Townsend]

No, super-review has not really been a thing for some time, we should
remove documentation suggesting it is. That said we definitely have room
for this kind of architectural review. Webidl for example already uses
something like this.

On Fri, Apr 20, 2018 at 2:24 PM Kris Maglione <kmag...@mozilla.com> wrote:

> I can't remember the last time I saw a super-review request, but
> it's still documented as a policy[1]. Is it still a thing? Do we
> want it to still be a thing?
>
> The reason that I ask is that I have a problem that I think I
> might be able to solve by co-opting the super-review flag, but I
> don't want to trample on it if we're still using it as
> originally designed.
>
> My problem is essentially that, for a few areas of code, I'm the
> final arbiter, or at least the main blocker, for a lot of large
> or critical architectural or API changes. The upshot of that is
> that I get a lot of review requests for patches in those areas,
> and most of the patches that make it into my queue are large
> and/or complicated. This situation gets exhausting after a
> while. And since people know that I tend to be busy, they also
> avoid flagging me to review smaller patches, even when I really
> *should* look at those patches (and therefore notice issues with
> them only when I read code later).
>

> For a lot of these patches, my opinion is only really critical
> for certain architectural aspects, or implementation aspects at
> a few critical points. There are other reviewers who are
> perfectly qualified to do a more detailed review of the
> specifics of the patch, and have more spare cycles to devote to
> it. Essentially, what's needed from me in these cases is a
> super-review, which I can do fairly easily, but instead I become
> a bottleneck for the code review as well.
>
> So, for the areas where I have this responsibility, I'd like to
> institute a policy that certain types of changes need a final
> super-review from me, but should get a detailed code review from
> another qualified reviewer when that makes sense.
>
>

> Does anyone have any objection to this plan? Or any suggestions
> for another way to go about it?
>
> -Kris
>
> [1]: https://www.mozilla.org/en-US/about/governance/policies/reviewers/

[Dave Townsend]

Presumably it supports multiple reviews for a patch, in which case I think
we're fine.

On Fri, Apr 20, 2018 at 3:03 PM Gregory Szorc <g...@mozilla.com> wrote:

> On Fri, Apr 20, 2018 at 2:51 PM, L. David Baron <dba...@dbaron.org> wrote:
>
> > On Friday 2018-04-20 14:23 -0700, Kris Maglione wrote:

> > > For a lot of these patches, my opinion is only really critical for
> > certain
> > > architectural aspects, or implementation aspects at a few critical
> > points.
> > > There are other reviewers who are perfectly qualified to do a more
> > detailed
> > > review of the specifics of the patch, and have more spare cycles to
> > devote
> > > to it. Essentially, what's needed from me in these cases is a
> > super-review,
> > > which I can do fairly easily, but instead I become a bottleneck for the
> > code
> > > review as well.
> > >
> > > So, for the areas where I have this responsibility, I'd like to
> > institute a
> > > policy that certain types of changes need a final super-review from me,
> > but
> > > should get a detailed code review from another qualified reviewer when
> > that
> > > makes sense.
> >

> > I think it's reasonable to use the super-review flag for this sort
> > of high-level or design review, at least until we come up with a
> > better name for it (and make a new flag, and retire the old one). I
> > don't think the super-review policy (as written) is meaningful
> > today.
>
>
> FWIW I'm pretty sure Phabricator won't support the super-review flag. And
> since we're aiming to transition all reviews to Phabricator...

[Eric Rescorla]

On Fri, Apr 20, 2018 at 7:03 PM, Dave Townsend <dtow...@mozilla.com>
wrote:

> Presumably it supports multiple reviews for a patch, in which case I think
> we're fine.
>

It does.

-Ekr

[Gijs Kruitbosch]

On 20/04/2018 22:23, Kris Maglione wrote:
> I can't remember the last time I saw a super-review request, but it's
> still documented as a policy[1]. Is it still a thing? Do we want it to
> still be a thing?

Not in the way it used to be. See
https://groups.google.com/forum/#!topic/mozilla.governance/HHU0h-44NDo .

The policy doc should have been retired / marked as historical. I don't
know how to go about making that happen... presumably talking to the
mozilla.org folks ?

> My problem is essentially that, for a few areas of code, I'm the final
> arbiter, or at least the main blocker, for a lot of large or critical
> architectural or API changes. The upshot of that is that I get a lot of
> review requests for patches in those areas, and most of the patches that
> make it into my queue are large and/or complicated. This situation gets
> exhausting after a while. And since people know that I tend to be busy,
> they also avoid flagging me to review smaller patches, even when I
> really *should* look at those patches (and therefore notice issues with
> them only when I read code later).
>
> For a lot of these patches, my opinion is only really critical for
> certain architectural aspects, or implementation aspects at a few
> critical points. There are other reviewers who are perfectly qualified
> to do a more detailed review of the specifics of the patch, and have
> more spare cycles to devote to it. Essentially, what's needed from me in
> these cases is a super-review, which I can do fairly easily, but instead
> I become a bottleneck for the code review as well.
>
> So, for the areas where I have this responsibility, I'd like to
> institute a policy that certain types of changes need a final
> super-review from me, but should get a detailed code review from another
> qualified reviewer when that makes sense.
>
>
> Does anyone have any objection to this plan? Or any suggestions for
> another way to go about it?

I'd like to ask 2 questions:

- which specific areas of code will this cover? The other examples cited
in this thread (webidl reviews; old-style super-review) have (had) clear
definitions of the boundaries of this type of oversight, and esp. the
former is enforced via commit/push hooks. Do you intend to do the same here?
- what are we/you doing to fix the bus factor here? Again, both of the
webidl and previous sr requirements have (had) groups of people
associated with them, so things weren't blocked if 1 person went on
holiday, left the project, or was otherwise not available. I think we
need to avoid ending up in a situation where you can't take holidays or
where we're completely stuffed if you ever decide to leave. :-)

Especially to the latter of these 2 points, I would feel more
comfortable with this arrangement if there was more than 1 person around
to exercise this type of oversight, even if you were the
"owner"/buck-stopper and other(s) were "peers" (in the module governance
sense).

~ Gijs

>
> -Kris
>
> [1]: https://www.mozilla.org/en-US/about/governance/policies/reviewers/

[Mark Côté]

A few comments on Phabricator and Lando:

Phabricator has two types of review requests: "reviewer" and "blocking reviewer". These are only really differentiated if there is more than one reviewer on a revision. In that case, if there is a blocking reviewer, the revision is only marked "accepted" when the blocking reviewer(s) have accepted it. If there are only nonblocking reviewers, the revision is marked accepted when any one accepts it, as long as no others have requested changes.

How we might use blocking reviewers in our workflow is still open, but it could be used for training up reviewers, in which case the trainee would be a regular reviewer and the module peer/owner would be a blocking reviewer.

We have some plans to add enforced reviewers to Lando, meaning a patch can only be landed if it has a review from an appropriate module peer or owner. This is still hazy right now because it requires some changes to the module system, most notably making it machine readable. There's a somewhat related thread in governance on this.

Mark

[Cameron McCormack]

On Thu, Apr 26, 2018, at 12:41 AM, Mark Côté wrote:
> How we might use blocking reviewers in our workflow is still open, but
> it could be used for training up reviewers, in which case the trainee
> would be a regular reviewer and the module peer/owner would be a
> blocking reviewer.

It's not uncommon for me to submit patches for review from multiple people where I require all reviewers to sign off on the patch, usually because I ask them to look at different parts of the patch. I don't think I have ever sent a patch to get review from more than one person with the intention of landing it if only one person OKs it. (I'd probably needinfo people to get that kind of feedback.) So ignoring potential new workflows like the training one you mention, I think I would exclusively use blocking reviewers. It's good to know that Phabricator will help me avoid accidentally landing patches when not all of my reviewers have signed off.

[Randell Jesup]

Agreed... but it sounds like they're not quite sure how this will be
used. I'm concerned that developers may be confused and just as for
review by N people, not realizing it can be landed when one of them
r+'s. (Especially if the developer doesn't frequently use multiple
reviewers.) At minimum, if this how it works, there will been to be
clear communication about when to use this - or (better!) to switch the
default to blocking, and have the unusual/more-work-to-ask-for case be
any-of.

Once in a blue moon I'll ask for a fast review from any of N people,
then cancel the r?'s once i have 1 r+. but this is really rare, and
mostly when there's a time deadline to land something ASAP (sec issue,
or to get in ahead of a merge date). 99% of the time when I ask for N
people, I need r+ from all of them.

On the other side, I do know that the build peers (IIRC) us a shared
reviewing setup, where most bugs are thrown in a pool for any of them to
review. But that's not the normal workflow for any other group I know
of in Mozilla, at least in Firefox. (I don't know them all, though.)

--
Randell Jesup, Mozilla Corp
remove "news" for personal email

[Gregory Szorc]

When you take a step back, I think you have to conclude that our current model of requesting reviews from *individuals* is not practical for the common case. The way our code governance model works is that groups of people (module members) are empowered to sign off on changes. But because existing review tooling (practically speaking) only allows assigning reviews to individuals (as opposed to a group consisting of module members), that’s what we do. I.e. tools are dictating sub-optimal workflows that don’t match our governance model.

In the common case, I think group assignment is preferred. Maybe it assigns to an available person at submission time. But the submitter should not need to be burdened with picking an individual. (This picking requirement is especially hostile to new contributors who don’t know who to pick for review.)

Phabricator has the concept of a normal “reviewer” and a “blocking reviewer.” “Blocking” means they must sign off before the review is marked as accepted.

I think the world we’ll eventually get to is one where most reviews are assigned to groups (ideally automatically by mapping files changed to appropriate reviewers) as normal non-blocking reviewers in Phabricator. For special cases, we can assign individuals for review and/or set “blocking reviewers” so someone or a member of a group *must* sign off. The review submission tooling should make all of this turnkey - allowing people to easily opt in to the special case if warranted. But most review requirement policies should be codified so tools can apply/enforce them without human intervention.

It’s worth noting that in a future world where the review tool isn’t constraining us to assigning reviews to individuals and where group assignment is the default, assignments to individuals may represent shortcomings in the distribution of expertise. I.e. if only one person is capable of performing a review, then we have a bus factor of 1 and there is a business continuity concern. Assigning reviews to groups - perhaps specialized groups like “dom experts” instead of the normal “dom peers” group - helps reinforce that reviewing - and the knowledge required to perform it - is a shared responsibility and isn’t limited to single individuals. This is another reason why I think it is a good idea to lean heavily towards group-based review over individual. Even if the group consists of a single person, a group reinforces that the responsibility is distributed.

I think that once review submission doesn’t require submitters to choose reviewers and reviews magically end up on the plates of appropriate people, we’ll all look back at our current workflow and view it as primitive.

[Eric Shepherd]

It seems that, yes, a model in which a patch to a given component would
need reviews by any one person in a specific pool of reviewers makes a lot
of sense. Even more so if the patch submitter doesn't have to figure out
who is in the pool.

Having a mechanism that supports a secondary, high-level review for cases
involving a lot of interrelated patches, architectural changes, UX updates,
etc. would also make a lot of sense. Call it a meta-review, perhaps; as in
reviewing a meta bug's set of changes.

--
Eric Shepherd
Senior Technical Writer
Mozilla Developer Network
https://developer.mozilla.org/
Blog: http://www bitstampede.com/
Twitter: https://twitter.com/sheppy

On May 10, 2018 at 11:15 AM, Gregory Szorc <gsz...@mozilla.com> wrote:



On May 10, 2018, at 06:47, Randell Jesup <rje...@jesup.org> wrote: