Super-review policy: recap
Mike Connor
see the archives (http://groups.google.com/group/mozilla.governance)
for the thread from January, but there was general consensus that the
proposal below felt right. There were some tweaks to the criteria to
reflect discussion around API changes, but otherwise this is generally
just a recap and a last call for feedback before we make the change
official.
====
The core principles behind these changes are that:
* The super-reviewers group is capable of making the biggest impact in
the areas of API and code design, security bugs, and testability/
maintainability
* All of the code in mozilla-central can benefit from this type of
review and guidance
* Module owner review, assuming strong ownership and ever-growing test
coverage, should be sufficient for many patches.
The proposal we have is that super-review will now be required for any
patches to mozilla-central meeting any of the following criteria:
* any significant architectural refactoring
* any change in how Mozilla code modules interact
* all security-related changes (including security bug fixes and
security model changes)
* any change to an API
** The definition of an API here is intentionally not crisp.
Traditional XPCOM API, objects/functions reflected in global scope,
public XBL bindings, and other things may all be considered to be APIs
for the purpose of this rule. Where it is unclear, the primary
reviewer will have to use their judgement.
Module owners/peers may ask for a super-review in other cases not
explicitly covered here, like any other conditional grant of review.
Any patch not meeting the above criteria does _not_ require super-
review under this proposal, even in areas where super-review has
previously been required for all patches.
IMPORTANT: This proposal covers browser and toolkit, which previously
had no requirement for super-review.
====
I'm going to plan on getting a draft together in the next week based
on these guidelines. If you missed the discussion last time, and you
have anything new to raise, please reply sooner rather than later.
-- Mike
Mike Connor
Mitchell Baker
Mike's proposal applies only to moz-central. I wonder about
comm-central (nothing that I understand calendar has not used
"super-review as it was previously defined)
mitchell
Mitchell Baker
Mike's proposal applies only to moz-central. I wonder about
comm-central (nothing that I understand calendar has not used
"super-review as it was previously defined)
mitchell
Robert Kaiser
> Personally I don't want super-review approval to be a prerequisite to get
> calendar commit access.
I object when it comes to *getting access* to the repo - rules for
actual commits are your team's problem when it comes to calendar/ files.
> The reasoning is as follows:
>
> - we only have two super-reviewers (Neil Rashbrook and David Bienvenu)
> who are actively working in comm-central right now.
Irrelevant, as sr isn't bound to specific modules.
> - The calendar crew is (unfortunately) very small.
Irrelevant, as the sr probably shouldn't even have done any review on
your code with the rules coming out of this thread.
> - We're confined to our space.
This doesn't matter because this is about getting the *access* to
messing around with all the code in the repository, not about what code
one is actually touching with those permissions. If we applied the same
to other repos, that would mean JS devs don't need sr to get access to
mozilla-central because they only touch JS code.
> - Nothing is broken.
Oh, but it is. Currently everyone with access to any hg repository can
change any comm-central code. I'm just glad most people don't know they
can. But: yes, they can. And that's seriously broken. I'd rather
restrict comm-central access to the same group that has mozilla-central
access than leave it the way it is indefinitely.
Robert Kaiser
Robert Kaiser
> Mike's proposal applies only to moz-central. I wonder about comm-central
> (nothing that I understand calendar has not used "super-review as it was
> previously defined)
I think it should apply to both (and of course the equivalent releases/
repositories), we want people to interact between the two repositories,
they should only be technically distinct, ideally not when it comes to
rules.
Robert Kaiser
Mike Connor
On 24-May-09, at 4:33 AM, Simon Paquet wrote:
> Mitchell Baker wrote:
>
>> Mike's proposal applies only to moz-central. I wonder about
>> comm-central (nothing that I understand calendar has not used
>> "super-review as it was previously defined)
>
> Personally I don't want super-review approval to be a prerequisite
> to get
> calendar commit access.
My proposal in this thread has nothing to do with commit access. This
is about changing review requirements on mozilla-central to make super-
review universal, but with specific scope...
-- Mike
Robert Kaiser
Sorry, disregard this, I was confused between the commit access and code
review topics.
Robert Kaiser
Dan Mosedale
to are where highly leveraged but somewhat conservative (in a certain
sense) development should happen, and that other development should
happen elsewhere.
mozilla-central is in the right position to do this now, because code
that didn't meet those criteria wasn't pulled forward from CVS.
comm-central is in a somewhat different state: for mail/ and mailnews/,
that policy sounds like a reasonable one to me (though I'd like to hear
other reviewers' thoughts on that as well). I'm a bit inclined to want
to staple that change to us getting some live metrics around review
turnaround, abandoned patches, and other project health stuff, because
making changes like this blindly doesn't strike me as good idea.
I think calendar/ is in a spot where (at the moment, at least), adding
more review process isn't really an option. My hope is that that will
change down the road a bit, but right now, it needs to have low overhead
much more than it needs to avoid mistakes.
And I can't speak for suite/ at all.
So I guess my inclination is that this sounds like a reasonable
long-term goal, but it's not something that's worth moving on immediately.
Dan
Dave Townsend
some of the docs on MDC seem to suggest it is.
Robert O'Callahan
and I especially don't like the requirement that all security bugs have
sr. I see no evidence that we will benefit from these additional review
requirements. But I ignored these threads earlier so I guess that's too
bad for me.
However, we should do something about this paragraph in reviewers.html:
> The JavaScript engine, like NSPR, is a closed CVS partition
> containing mature code, with dependencies only on another closed CVS
> partition (NSPR). Module owner review suffices to change JS code.
JS is by no means "mature code". This should state the real reasons JS
is exempt from super-review, or just be silent about the reasons.
Rob
Mike Connor
> I don't like the new requirement that sr and r not be the same
> person, and I especially don't like the requirement that all
> security bugs have sr. I see no evidence that we will benefit from
> these additional review requirements. But I ignored these threads
> earlier so I guess that's too bad for me.
Really, the old policy never allowed for r+sr, but we tolerated it in
some areas to keep things moving. I think that the limited scope of
SR, and what should be a significant reduction in the number of SR
requests after non-SR module owner reviews, should offset any
perceived increase in scope. I think that security bugs in particular
we should have multiple reviews, out of any set of bugs we fix these
are the ones that warrant the most collaboration, IMO.
> However, we should do something about this paragraph in
> reviewers.html:
>
> > The JavaScript engine, like NSPR, is a closed CVS partition
> > containing mature code, with dependencies only on another closed CVS
> > partition (NSPR). Module owner review suffices to change JS code.
>
> JS is by no means "mature code". This should state the real reasons
> JS is exempt from super-review, or just be silent about the reasons.
And that real reason is? This exception predates my involvement in
the project (2003), so I really don't know.
-- Mike
Gervase Markham
>> JS is by no means "mature code". This should state the real reasons JS
>> is exempt from super-review, or just be silent about the reasons.
>
> And that real reason is? This exception predates my involvement in the
> project (2003), so I really don't know.
Also, how much development still goes on in the JS partition in CVS?
Isn't all the new JS engine hotness in various trees in Hg, which do not
have special checkin restrictions?
Gerv
Boris Zbarsky
> On Friday 2009-07-17 02:22 -0400, Mike Connor wrote:
>> in scope. I think that security bugs in particular we should have
>> multiple reviews, out of any set of bugs we fix these are the ones that
>> warrant the most collaboration, IMO.
>
> for security, it's the ones that we think are likely to *cause*
> security bugs, not the ones that fix those security bugs.
In some components (XPConnect comes to mind) the latter are typically
also the former...
I agree that this is not the case in layout, though. But it makes sense
to get extra review on security fixes to make sure they're not too
narrow, for example.... (say just patching symptoms, not causes).
-Boris
Mike Connor
This is the intent. And the policy covers _both_, so this really isn't
an either/or.
-- Mike
Robert O'Callahan
> Really, the old policy never allowed for r+sr, but we tolerated it in
> some areas to keep things moving. I think that the limited scope of SR,
> and what should be a significant reduction in the number of SR requests
> after non-SR module owner reviews, should offset any perceived increase
> in scope. I think that security bugs in particular we should have
> multiple reviews, out of any set of bugs we fix these are the ones that
> warrant the most collaboration, IMO.
Getting two independent reviews for layout patches for maybe-exploitable
crashes is going to be onerous. It certainly means more work for me,
dbaron and bz. Was any kind of cost-benefit analysis done here?
>> However, we should do something about this paragraph in reviewers.html:
>>
>> > The JavaScript engine, like NSPR, is a closed CVS partition
>> > containing mature code, with dependencies only on another closed CVS
>> > partition (NSPR). Module owner review suffices to change JS code.
>>
>> JS is by no means "mature code". This should state the real reasons JS
>> is exempt from super-review, or just be silent about the reasons.
>
> And that real reason is? This exception predates my involvement in the
> project (2003), so I really don't know.
I don't know. You'd have to ask Brendan I guess.
Rob
Robert O'Callahan
> I agree that this is not the case in layout, though. But it makes sense
> to get extra review on security fixes to make sure they're not too
> narrow, for example.... (say just patching symptoms, not causes).
It's going to be a pain to find two reviewers for any maybe-exploitable
crash in, say, block-and-line reflow.
Rob
Mike Beltzner
>> I thought sr wasn't module specific?
I asked Connor about this on Friday, he clarified that sr is not
module specific.
> Do you think that super-review from a reviewer who isn't familiar with
> layout code is going to add value when one of roc/bz/dbaron have
> already reviewed? It seems unlikely to me.
I asked Connor about this on Friday, too, and his assertion was that
sr is less about making sure that the specific code does what it's
supposed to do, and more about looking for common patterns which are
known to be less effective than others at preventing crashes, or more
likely than others to have bad effects on other areas of the codebase.
As a driver, I do fear that this sr requirement is adding yet another
rubber stamp, but agreed that we should see how it goes. If it turns
out to be especially onerous, we can change the policy. These things
aren't set in unbreakable concrete.
cheers,
mike
Mike Shaver
> As a driver, I do fear that this sr requirement is adding yet another rubber
> stamp, but agreed that we should see how it goes. If it turns out to be
> especially onerous, we can change the policy. These things aren't set in
> unbreakable concrete.
It would be good, for changes to policy, to know what motivates the
change. As an sr and driver, I don't think that the review
requirements for layout have been a problem in need of a solution.
Are we seeing a lot of bugs or otherwise-undesirable code (poor style,
fragile dependencies, etc.) in layout that we believe would be caught
by super-review? I wouldn't have said so, and I don't think the
module owners were asking for such help, but I haven't looked at the
data.
What problems are we addressing with the changes to policy? How will
we know if they worked, or if we need to change the policy again at
some point?
Mike
Mike Connor
On 20-Jul-09, at 8:30 AM, Mike Shaver wrote:
> On Mon, Jul 20, 2009 at 7:54 AM, Mike Beltzner<belt...@mozilla.com>
> wrote:
>> As a driver, I do fear that this sr requirement is adding yet
>> another rubber
>> stamp, but agreed that we should see how it goes. If it turns out
>> to be
>> especially onerous, we can change the policy. These things aren't
>> set in
>> unbreakable concrete.
>
> It would be good, for changes to policy, to know what motivates the
> change. As an sr and driver, I don't think that the review
> requirements for layout have been a problem in need of a solution.
> Are we seeing a lot of bugs or otherwise-undesirable code (poor style,
> fragile dependencies, etc.) in layout that we believe would be caught
> by super-review? I wouldn't have said so, and I don't think the
> module owners were asking for such help, but I haven't looked at the
> data.
The motivations have been discussed repeatedly, in multiple threads in
which you have given feedback, but sure, let's play this game. The
motivations were to reform the super-review process, and continue to
apply it where it had high value, and to no longer apply it where it
was deemed to add insufficient value. (More on specifics below.)
Given the changes in scope, the policy change _should_ result in less
time spent on super-review of patches overall. [1] That is one of the
original reasons I proposed dropping the SR requirement for the
majority of patches, as I hope you recall. I recognize that in some
cases (i.e. block-and-line reflow in layout) this may require more
time for that specific area, but at the same time, I don't actually
think that's harmful. If we have a module that has enough possibly-
exploitable crashes that review bandwidth is a concern, I think the
real problem is that the code is fragile and needs both more careful
review and better code design, which seems precisely the type of issue
super-reviewers should be working against...
> What problems are we addressing with the changes to policy? How will
> we know if they worked, or if we need to change the policy again at
> some point?
My data isn't as hard as it could be, but I don't think you really
expect me to do stats on patterns that have proven themselves to be
common over the last four major cycles. As a driver who's lived in
Bugzilla for years, these patterns recur in obvious enough ways that I
don't feel digging up the hard data is an effective use of anyone's
time.
Key goals:
* Focus SR resources on key aspects of patches that matter most and
leave the rest to module owners
** To monitor (somewhat subjective): overall time spent on SR for
patches should decrease, since fewer bugs will require SR and of those
that remain, only specific aspects are in the scope of the SR.
* Reduce API churn and exert better control over late-breaking API
changes
** To monitor: fewer last-minute compat-saving fixes, better response
from extension authors.
* Reduce regression/incomplete fix rates on security fixes/arch changes
** To monitor: fewer regressions in stable releases, fewer followup
bugs/patches fixing additional cases not covered by the original patch.
* Improve overall code architecture for maintainability
** To monitor (kinda subjective): Fewer scary areas in new/refactored
code, easier/better maintenance and test authorship, broader automated
test coverage
I think these are still obvious and major pain points, for which we do
not currently have effective mitigation in the form of automated test
coverage (it helps, but is not a panacea). If you disagree that these
are problems worth solving, or that super-reviewers is the best group
to drive these consistently across the codebase, we should have that
conversation more explicitly.
-- Mike
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=423132 is an ideal
example of a super-review under this policy, which was signing off on
the API changes and general approach as opposed to an in-depth review
of the code changes. Relatively fast and easy, easily 1/4 or less the
amount of time I would have spent before the policy change.
Gavin Sharp
> less about making sure that the specific code does what it's supposed to do,
> and more about looking for common patterns which are known to be less
> effective than others at preventing crashes, or more likely than others to
> have bad effects on other areas of the codebase.
Right, I understand that. But in the specific case we're discussing,
all of the reviewers for the code are super-reviewers, and I think in
practice the current review requirements have been sufficient, so it's
harder to see the benefit to the additional requirements imposed by
the new policy.
My initial reaction to the thought of requiring additional review from
a super-reviewer on a significant number of the patches I review to
front-end code (in browser/ and toolkit/) was that it would be an
onerous bottleneck added to a process that already isn't quite
optimal. There aren't many super-reviewers who typically review
front-end code, and fewer still who have it as one of their top
priorities, so it's not too hard to see the potential for additional
frustrating and unnecessary delays.
Maybe I'm just overly optimistic about the current state of code
review, I guess? I'm not entirely comfortable raising this argument
since it is based primarily on the assertion "(we are/I am) already
doing a good enough job", and such arguments are somewhat difficult to
make objectively :)
Gavin
Clint Talbert
> Key goals:
>
> * Improve overall code architecture for maintainability
> ** To monitor (kinda subjective): Fewer scary areas in new/refactored
> code, easier/better maintenance and test authorship, broader automated
> test coverage
>
> I think these are still obvious and major pain points, for which we do
> not currently have effective mitigation in the form of automated test
> coverage (it helps, but is not a panacea). If you disagree that these
> are problems worth solving, or that super-reviewers is the best group to
> drive these consistently across the codebase, we should have that
> conversation more explicitly.
>
hear your thoughts on where we are missing test coverage, and in what
areas we need to broaden that coverage. We're trying to address several
of those over the next small number of weeks and months, and I want to
be sure that I hear as much feedback on that as I can get so that we
aren't building any "bridges to nowhere".
Please email me directly or start a thread with your thoughts. (Perhaps
in mozilla.dev.quality?)
Thanks Connor.
Clint
Mike Connor
On 20-Jul-09, at 1:31 PM, Gavin Sharp wrote:
> On Mon, Jul 20, 2009 at 7:54 AM, Mike Beltzner<belt...@mozilla.com>
> wrote:
>> I asked Connor about this on Friday, too, and his assertion was
>> that sr is
>> less about making sure that the specific code does what it's
>> supposed to do,
>> and more about looking for common patterns which are known to be less
>> effective than others at preventing crashes, or more likely than
>> others to
>> have bad effects on other areas of the codebase.
>
> Right, I understand that. But in the specific case we're discussing,
> all of the reviewers for the code are super-reviewers, and I think in
> practice the current review requirements have been sufficient, so it's
> harder to see the benefit to the additional requirements imposed by
> the new policy.
With the exception of the r+sr case, and browser/toolkit, we've
reduced the scope of requirements, not increased them, at least on
paper. That said, it seems like r+sr has become more prevalent/
expected than I realized where peers are largely SRs, so that bears
watching. Still should net positive for everyone, but I realize that
it might not seem that way on the surface.
> My initial reaction to the thought of requiring additional review from
> a super-reviewer on a significant number of the patches I review to
> front-end code (in browser/ and toolkit/) was that it would be an
> onerous bottleneck added to a process that already isn't quite
> optimal. There aren't many super-reviewers who typically review
> front-end code, and fewer still who have it as one of their top
> priorities, so it's not too hard to see the potential for additional
> frustrating and unnecessary delays.
I'm an SR, vlad's an SR, bsmedberg's an SR. If that's not enough,
we'll find out soon enough, and we'll find a way to solve that. (My
review queue fits on screen without scrolling now, for the first time
since 2005, so I don't have excuses for not getting to SRs in a timely
fashion.) Beyond that, the goal in reducing scope of SR was to free
up cycles, some of which were intended to help browser/toolkit.
> Maybe I'm just overly optimistic about the current state of code
> review, I guess? I'm not entirely comfortable raising this argument
> since it is based primarily on the assertion "(we are/I am) already
> doing a good enough job", and such arguments are somewhat difficult to
> make objectively :)
I think that in the areas of API design and code architecture we've
had some notable failings in the front end over the last six years. I
think in general terms we're doing very well, but the places we aren't
(and I've been a peer or better since 2004ish, so it mostly happened
on my watch) is where we've added SR going forward as an added
requirement. This should not be taken as a slight against
individuals, but I believe there's some collective failures that we
need to correct and learn how to avoid in the future.
-- Mike
Robert O'Callahan
> I recognize that in some cases (i.e.
> block-and-line reflow in layout)
Pretty much all of layout, actually.
> this may require more time for that
> specific area, but at the same time, I don't actually think that's
> harmful. If we have a module that has enough possibly-exploitable
> crashes that review bandwidth is a concern, I think the real problem is
> that the code is fragile and needs both more careful review and better
> code design, which seems precisely the type of issue super-reviewers
> should be working against...
It absolutely true that block-and-line in particular and layout in
general is fragile and poorly designed. I have been thinking, talking
and blogging about that for many years. We are gradually improving, but
it's a huge project.
Additional super-review of particular bug fixes is not going to shed any
new light on these problems. It will, however, slow us down a little bit.
Rob
Mike Connor
Do you think super-review has a place at all? Is layout a special
case because some of the peers are also SRs? What's the cost of the
SR (keeping in mind the relatively limited scope) vs. the benefit of
having more input on the changes as they happen?
-- Mike
Mike Shaver
> Do you think super-review has a place at all? Is layout a special case
> because some of the peers are also SRs? What's the cost of the SR (keeping
> in mind the relatively limited scope) vs. the benefit of having more input
> on the changes as they happen?
My observation of layout bugs indicates that when a second review is
valuable, due to limitations in the experience of the primary reviewer
or due to the gross complexity or scale of the patch, they ask for a
second review. That's independent of whether the first reviewer was
also an SR.
That is to say, r+sr isn't automatic or necessarily itself sufficient
for any given patch, and layout has been quite successful so far in
matching review count (and personnel; the second reviewer might not be
an sr) to patch needs. I wouldn't want people to pick a second
reviewer who happened to be SR over one who isn't but has domain
knowledge, because they need the SR bit anyway and don't want to go
through 3 reviews. That's a behaviour for which we're creating an
incentive here, and I am a big believer in incentives!
(Thanks for the recap of the motivations; I'm sure they've been
pointed out in other threads, but it's hard to keep them all in my
head over the course of multiple threads across multiple weeks.
Having refreshed them, I don't see how forbidding r+sr moves them
forward, though.)
Mike
Robert O'Callahan
> Do you think super-review has a place at all? Is layout a special case
> because some of the peers are also SRs? What's the cost of the SR
> (keeping in mind the relatively limited scope) vs. the benefit of having
> more input on the changes as they happen?
I think additional review is often useful at the design and (internal or
external) API level. When I file a bug proposing some significant change
in those areas, I tend to CC everyone who I think might have some
insight or interest. Requesting an additional formal SR makes sense here
in many cases, but would be overkill for some small cases. And often the
best way to get design review is not SR on a patch but a blog post,
newsgroup post, etc.
Security-related design is covered by the above. It's the SR requirement
for any "security bug", even if it's a mundane code change, that I find
particularly onerous.
I don't think layout is particularly special. I think in most Gecko
modules (at least weighted by activity) we have owners and peers who
either are sr's or who could be.
The cost here is a slowdown in getting patches into the tree and
people's time doing the SRs --- people who are already generally
swamped, and who are able to use their time in very valuable ways elsewhere.
If you ask me, instead of having strict rules about what requires SR, I
would rely on patch-by-patch judgement of the author and primary
reviewer, with after-the-fact discipline when we see incidents of
insufficient review. If module owners and peers, at least, are unable to
learn appropriate behaviour then we have a problem that cannot be solved
by rules.
I see a lot of good outreach for design and API review happening,
though. (E.g., bug 753.) True, the cases where it's not happening are
exactly the cases that are invisible, but we can still be vigilant about
what's going into the tree.
Rob
fantasai
I think it makes sense to allow r+sr for security bugs if they don't fall
under any of the other criteria that require sr.
So, What needs super-review? would be
* Significant architectural refactoring
* Any change to any API or pseudo-API (internal or external)
* All changes that affect how code modules interact
* Changes to security models or mechanisms
* All security bugs (r+sr OK if no other category triggered)
~fantasai