Finer-grained OWNERS (2014 edition)

[Philip Rogers]

You all rock and our core/owners[1] list has almost doubled since our last discussion[2] about finer-grained owners files in April 2013! In light of how core/owners is being used I would like to revisit the discussion with the following changes:

* Add owners files for the subdirectories that have turned out to be too coupled with core to become modules but still have implicit owners. These sub-owners files would follow the standard chromium owners policy outlined in http://www.chromium.org/developers/owners-files.

* Add comments to the current owners file where sub-owners files are not appropriate. This will clean up some unnecessary friction when finding a good reviewer. Owners should TBR changes to add comments above their name.

I’ve uploaded a trial balloon for these changes based on subdirectory commit counts over the past year[3]:

https://codereview.chromium.org/625053003

[1] https://chromium.googlesource.com/chromium/blink/+/master/Source/core/OWNERS

[2] https://groups.google.com/a/chromium.org/d/msg/blink-dev/qx5_z_g3eL0/vPYq8uHqF7EJ

[3] https://docs.google.com/a/chromium.org/spreadsheets/d/13DgGH9jbcHO5yRTOIYs6LBW5C1vDXYccrKfK6hbm7DE/view

[Ian Vollick]

I've found a shortage of owners in web/ to be a bottleneck. Any chance we could revisit our policy for ownership there, too? (Would it be crazy to look at public/* ownership, too?)

[Adam Barth]

On Fri Oct 03 2014 at 2:28:38 PM Philip Rogers <p...@chromium.org> wrote:

You all rock and our core/owners[1] list has almost doubled since our last discussion[2] about finer-grained owners files in April 2013! In light of how core/owners is being used I would like to revisit the discussion with the following changes:

* Add owners files for the subdirectories that have turned out to be too coupled with core to become modules but still have implicit owners. These sub-owners files would follow the standard chromium owners policy outlined in http://www.chromium.org/developers/owners-files.

I think this approach makes a lot of sense.  I don't think the core/owners approach has scaled particularly well as the project has grown.

Another thing we might want to consider is reorganizing the directory structure within core to better match areas of expertise.  For example, the core/events directory is a grab-bag of events related to lots of different subsystems.  Maybe we should move the events related to a feature closer to the feature, even if its not in a module?  These sorts of reorganizations are easier than factoring out a module because they don't involve restructuring dependencies.

* Add comments to the current owners file where sub-owners files are not appropriate. This will clean up some unnecessary friction when finding a good reviewer. Owners should TBR changes to add comments above their name.

In other parts of Chrome, people are listed redundantly in finer grained owners files to make it easier for folks to find appropriate reviewers.

On Fri Oct 03 2014 at 5:24:20 PM Ian Vollick <vol...@chromium.org> wrote:

I've found a shortage of owners in web/ to be a bottleneck. Any chance we could revisit our policy for ownership there, too? (Would it be crazy to look at public/* ownership, too?)

I'd recommend starting threads with the owners of those directories.  There's no special policy for those directories (unlike core), which means the owners of those directories are in charge of them.

Adam

[Kouhei Ueno]

+1 to the idea.

As a committer outside of PST, it would be nice if we could keep the timezone distribution of OWNERS in the new scheme.

It is always nice to have a local OWNER for better throughput.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

--

Kouhei Ueno

[Ojan Vafai]

+1 to the idea overall. I wonder if we want to have some folders where we don't allow sub owners files though. For example, it's hard for me to see breaking up rendering, css or dom. You really have to have a good grasp of the whole directory to do reviews for any part of it.

[Jeremy Roman]

Seems like a good case for comments, so that it's easier to tell which reviewers are best for DOM, CSS, etc. Even if that list remains in core/OWNERS.

[Ojan Vafai]

I was a little too vague. I was picturing that we'd have rendering/OWNERS, but not rendering/svg/OWNERS.

[Noel Gordon]

On 4 October 2014 14:59, 'Kouhei Ueno' via blink-dev <blin...@chromium.org> wrote:

As a committer outside of PST, it would be nice if we could keep the timezone distribution of OWNERS in the new scheme. It is always nice to have a local OWNER for better throughput.

Concur.

[Eric Seidel]

Blink is merging with Chromium. I'd be interested in seeing a
proposal to adopt Chromium's OWNERS model wholesale (no restrictions
on sub-folder owners).

We have no APIs between our sub-components. That's going to make
fine-grained OWNERS hard (hard to change anything in rendering/ w/o
also being an OWNER in all directories which use rendering/).

I'd like to see a specific proposal to add OWNERS files (and maybe
empty core/OWNERS at the same time?). I'd start with one OWNER per
directory and let them grow the OWNERS list for that directory.

[Philip Rogers]

Eric,

Source/core is still too coupled to remove the core owners file, but I see this proposal as moving towards your goal. Are you suggesting we take a more extreme approach today?

[Julien Chaffraix]

On Mon, Oct 6, 2014 at 10:20 AM, Eric Seidel <ese...@chromium.org> wrote:

> Blink is merging with Chromium. I'd be interested in seeing a
> proposal to adopt Chromium's OWNERS model wholesale (no restrictions
> on sub-folder owners).

It seemed to be what was proposed by Philip. If we have no concept of
core OWNERS then we need some model akin to Chromium to handle the
sub-directory owners.

> We have no APIs between our sub-components. That's going to make
> fine-grained OWNERS hard (hard to change anything in rendering/ w/o
> also being an OWNER in all directories which use rendering/).
>
> I'd like to see a specific proposal to add OWNERS files (and maybe
> empty core/OWNERS at the same time?). I'd start with one OWNER per
> directory and let them grow the OWNERS list for that directory.

Removing core/OWNERS doesn't seem like a workable proposal at the
moment due to the lack of API. If we can get our directory structure
to be better aligned with the API consumers or get sub-components more
tightened, it may be possible but we shouldn't tie finer grained
OWNERS to these tasks.

Julien

[Jochen Eisinger]

I'd be fine with Eric's proposal, given that we preserve the list of core/OWNERS in some form, since it also serves as a list of outstanding contributors (similar to the reviewers in WK), and I think we should keep and maintain this list.

In chromium, there aren't that many APIs between components either. I'd expect that over time, if you e.g. can't change a file in directory A without touching a file in directory B, the list of owners for A and B will converge.

Given that we have Source/OWNERS sitting close to all timezones, I don't think remote offices will have problems finding somebody to review the occasional CL. And if find yourself constantly working on a component, maybe you should just become an owner of that.

best

-jochen

[Emil A Eklund]

The lack of active owners for Source/web and public/ is starting to
become I real problem. I really think we should prune inactive owners
from all owners files (or at the very least mark inactive owners as
emeritus) and add a new set of active ones.

Having inactive owners in the owners files is not only
counterproductive it is actually harmful in that they don't
necessarily have an up-to-date view of the world. It also slows things
down for everyone.

+1 for finer grained OWNERS files and for pruning inactive owners.

[David Dorwin]

+1

There are only 7 owners of public/ and a couple have recently indicated they are too busy to review specific CLs. If we assume darin@ isn't doing many such reviews, that leaves only four owners.

On the other end of the spectrum, there are 62 owners of core with no indication of who would be an appropriate reviewer for a given area. For example, who should review a media-related CL? (There is an answer, but you might only know this if you've been working on media.)

It's also difficult for new contributors to find an appropriate owner. Over time, you learn who is appropriate and what timezone they are in, but this can also skew the load towards those who have been owners the longest. Some ideas in addition to what has already been mentioned:

• Make it clear who is a reviewer and who are "Just owners" (i.e. example in Chromium) or otherwise unlikely to be the right choice for most CLs.
  
• Update the Chromium tooling to understand such comments. For example, the Chromite Butler extension seems to suggest anyone that could approve a CL, including root owners that are probably not appropriate.
• List a timezone or region next to each owner to make it easier to find an owner that is likely to be available (especially important if a second review is required).

David

[Eric Seidel]

I think we should consider removing all reviewers/OWNERS who have not
reviewed or committed within 3-6 months.

The public/OWNERS just had a separate (private) thread about
adding/replacing OWNERS in that file. If you're interested in being
an OWNER for public/ please contact one of those OWNERS.

Ideally if we have no active OWNERS it should be obvious by the lack
of lines in the file, rather than just a bunch of zombie OWNERS.

[Hajime Morrita]

This might be tangential/offtopic, but I feel that the short of {public|web|platform} reviewers might be a symptom of some design problems.

For actively developed and modules/-tied features, the chrome-side consumers of these API are ones who wrote the API. The strict API boundary makes less sense in that case. We should be able to just let them proceed. What's wrong here is, IMO, the skewed directory hierarchy and the lack of modularity caused by that. 

What's possible then? For example, we could have public/ for each modules (and even for core subsystems), say Sources/modules/foo having Sources/modules/foo/public/, which is owned by OWNERS of corresponding subsystems. Probably we need to add Source/ to the include path to make this work. That's ugly, but we can have finer-tuned DEPS on chrome side on the other hand, which is probably good.

I don't say this is the best idea, but my point here is that we don't have to have big, monolithic API layer. I think it's better to have finer grained set of APIs that each team is responsible for. I agree that there are certain API surface where we need safeguards, but I guess that that "strict" API can be much smaller subset of what we have today.

And optimistically speaking, giving each module owner to the authority will encourage us to make the strict API smaller by spinning features out to subsystems, that can result better layering.

WDYT?

[Jochen Eisinger]

I fear you'll just run against the even smaller group of content/public OWNERS... I would expect that for the majority of cases, using mojo and by that just skipping all of this is the more promising approach (at least long term)

[Hajime Morrita]

That's true, and I'm indeed running against content/public OWNERS these days  ;-)

[Elliott Sprehn]

The problem here is that modules don't have any real API boundary with core, so we allow a module to expose any new API it wants to Chromium you'll soon find all kinds of innards of core (like rendering and DOM) exposed in ways we didn't intend.

We really need stricter deps and API boundaries to allow that.

[Philip Rogers]

It sounds like there is support for this proposal so lets go ahead with landing this change today. Please follow http://www.chromium.org/developers/owners-files for subowners.

There were issues with Source/public and Source/web brought up in this thread. We just added Haraken as a Source/web owner in r183462 and the current Source/public owners are going to add additional owners as well.

There was also some useful discussion about future changes we'd like to make:

1) Reorganizing the directory structure to better reflect areas of expertise.

2) Cleanup of stale owners.

3) Removing Source/core owners completely!

[Yutaka Hirano]

Hi,

We created core/streams a few months ago, and Takeshi once asked for the subdirectory OWNER[1].

As we currently have subdirectory owners, I would like to ask for it again.

Thanks,

1: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/qx5_z_g3eL0/7Q18UjKOpgUJ

[Kouhei Ueno]

core/streams +1 given the current policy of no cross module dependencies.

Regarding the other therad, I agree that current XHR is misplaced in core/xml. However I would prefer it to be a module/ rather than core/.

XHR currently depends on other core/, but nothing in core/ depends on XHR afaik.

--

Kouhei Ueno

[Jeremy Roman]

How do people feel about sub-directory owners, one year later?

I've found the places that have sub-directory OWNERS useful (I've pointed people to core/html/parser/OWNERS when they were looking for parser experts, looked to core/animation/OWNERS to remember who owns animation, core/editing/OWNERS for editing, etc.), and wish it existed in other places that were reasonable (for instance, I have a mental list of CSS experts, but an explicit core/css/OWNERS would be nice).

Even if they are just subsets of core/OWNERS and platform/OWNERS (i.e. they are purely informational), I'd like to suggest that other directories in Blink adopt a similar policy. (In particular, I'm interested in creating core/paint/OWNERS and platform/graphics/OWNERS.)

For instance, when making a change to painting code I may wind up changing both core/paint and core/layout, but get a review from chrishtr because I know that the code is logically a paint change. Had I not already known that Chris is a good reviewer for that code, a core/paint/OWNERS file would give me a better starting point than core/OWNERS does.

Thoughts?

[Chris Harrelson]

Makes sense to me. +1

[Elliott Sprehn]

On Wed, Sep 16, 2015 at 1:29 PM, Jeremy Roman <jbr...@chromium.org> wrote:

How do people feel about sub-directory owners, one year later?

I've found the places that have sub-directory OWNERS useful (I've pointed people to core/html/parser/OWNERS when they were looking for parser experts, looked to core/animation/OWNERS to remember who owns animation, core/editing/OWNERS for editing, etc.), and wish it existed in other places that were reasonable (for instance, I have a mental list of CSS experts, but an explicit core/css/OWNERS would be nice).

Even if they are just subsets of core/OWNERS and platform/OWNERS (i.e. they are purely informational), I'd like to suggest that other directories in Blink adopt a similar policy. (In particular, I'm interested in creating core/paint/OWNERS and platform/graphics/OWNERS.)

As long as it's just subsets of core/OWNERS this seems fine. I don't think we should be adding OWNERS for directories for people that are not core owners though.

- E 

[Peter Kasting]

In Chrome we do this quite often, in part because we don't really have a concept of many people having core ownership, but also in part because often people's domain knowledge is specialized and it's appropriate to have someone in a narrow directory but not the wider root.

I actually am not a good core owner in Blink.   I'm listed in Source/core/OWNERS, but I really shouldn't do code reviews for non-image decoding stuff.  When it comes to that stuff, in certain areas I'm the expert, so it wouldn't be appropriate to remove me from OWNERS entirely.  It would be best to simply list me in the right subdirectories and not a root file.

I think Blink's current idea of having many people in core OWNERS files is a mistake, because it doesn't really represent how people's knowledge and abilities actually map to the code organization.

PK

[Elliott Sprehn]

On Wed, Sep 16, 2015 at 3:08 PM, Peter Kasting <pkas...@google.com> wrote:

...

In Chrome we do this quite often, in part because we don't really have a concept of many people having core ownership, but also in part because often people's domain knowledge is specialized and it's appropriate to have someone in a narrow directory but not the wider root.

I actually am not a good core owner in Blink.   I'm listed in Source/core/OWNERS, but I really shouldn't do code reviews for non-image decoding stuff.

You're welcome to remove yourself from core/OWNERS and just ask for rubber stamps instead.

 

When it comes to that stuff, in certain areas I'm the expert, so it wouldn't be appropriate to remove me from OWNERS entirely.  It would be best to simply list me in the right subdirectories and not a root file.

I think Blink's current idea of having many people in core OWNERS files is a mistake, because it doesn't really represent how people's knowledge and abilities actually map to the code organization.

Our system requires a higher bar, and necessarily so because the web platform API is very vast and the complexity of the engine is large. Indeed letting you be an owner in paint/ doesn't mean you understand the DOM/Layout or Style APIs, but it lets you review code using it. Maybe someday if we have rigid API boundaries we can have more relaxed owners. There's currently a project to lock down the API between Layout and Line Layout which is the first step in this direction.

- E

[Peter Kasting]

On Wed, Sep 16, 2015 at 3:18 PM, Elliott Sprehn <esp...@chromium.org> wrote:

On Wed, Sep 16, 2015 at 3:08 PM, Peter Kasting <pkas...@google.com> wrote:

...

In Chrome we do this quite often, in part because we don't really have a concept of many people having core ownership, but also in part because often people's domain knowledge is specialized and it's appropriate to have someone in a narrow directory but not the wider root.

I actually am not a good core owner in Blink.   I'm listed in Source/core/OWNERS, but I really shouldn't do code reviews for non-image decoding stuff.

You're welcome to remove yourself from core/OWNERS and just ask for rubber stamps instead.

The issue is that as I understand your proposal, this would disallow me from still being listed in deeper OWNERS files.  I think that consequence would be worse than what we have today.

When it comes to that stuff, in certain areas I'm the expert, so it wouldn't be appropriate to remove me from OWNERS entirely.  It would be best to simply list me in the right subdirectories and not a root file.

I think Blink's current idea of having many people in core OWNERS files is a mistake, because it doesn't really represent how people's knowledge and abilities actually map to the code organization.

Our system requires a higher bar, and necessarily so because the web platform API is very vast and the complexity of the engine is large.

I'm not sure I understand the counterargument.  I'm suggesting having fewer core OWNERS, not more.  The bar to be in some root-level OWNERS file should be higher than it is, not lower.  Coupled with that, I wouldn't gate that decision on putting someone in a leaf directory OWNERS file on whether they would be appropriate for one of the core files.  The result of these two is that more people should be in more non-core OWNERS files, fewer in core files, and engineers looking for reviews should generally not look in core files unless implementing changes that couch large cross-sections of the codebase.

PK

[Elliott Sprehn]

On Wed, Sep 16, 2015 at 3:27 PM, Peter Kasting <pkas...@google.com> wrote:

On Wed, Sep 16, 2015 at 3:18 PM, Elliott Sprehn <esp...@chromium.org> wrote:

On Wed, Sep 16, 2015 at 3:08 PM, Peter Kasting <pkas...@google.com> wrote:

...

When it comes to that stuff, in certain areas I'm the expert, so it wouldn't be appropriate to remove me from OWNERS entirely.  It would be best to simply list me in the right subdirectories and not a root file.

I think Blink's current idea of having many people in core OWNERS files is a mistake, because it doesn't really represent how people's knowledge and abilities actually map to the code organization.

Our system requires a higher bar, and necessarily so because the web platform API is very vast and the complexity of the engine is large.

I'm not sure I understand the counterargument.  I'm suggesting having fewer core OWNERS, not more.  The bar to be in some root-level OWNERS file should be higher than it is, not lower.  Coupled with that, I wouldn't gate that decision on putting someone in a leaf directory OWNERS file on whether they would be appropriate for one of the core files.  The result of these two is that more people should be in more non-core OWNERS files, fewer in core files, and engineers looking for reviews should generally not look in core files unless implementing changes that couch large cross-sections of the codebase.

If you're not trusted enough to be in core/OWNERS, you're not trusted enough to be in core/layout/OWNERS since you have access to all the same API surface there. Just adding people who work on a silo has not worked historically, it just means that slio'ed team dumps lots of code into their silo touching the rest of the system since it makes reviews easier.

To address this we're adding API boundaries, better ASSERTs, and include guards. Then we can discuss adding fine grained owners. :)

- E

[Peter Kasting]

On Wed, Sep 16, 2015 at 3:40 PM, Elliott Sprehn <esp...@chromium.org> wrote:

If you're not trusted enough to be in core/OWNERS, you're not trusted enough to be in core/layout/OWNERS since you have access to all the same API surface there. Just adding people who work on a silo has not worked historically, it just means that slio'ed team dumps lots of code into their silo touching the rest of the system since it makes reviews easier.

To address this we're adding API boundaries, better ASSERTs, and include guards. Then we can discuss adding fine grained owners. :)

We may be saying the same thing, and it's a question of how to get there, or how to set up the processes correctly.

We've definitely seen times in Chrome outside Blink where a silo'ed team has a negative effect on the product because they basically review each others' code and commit to their project without external oversight helping instill team culture, best practices, etc.  This was part of the original motivation for OWNERS, so that we'd ensure an area's OWNER was a reviewer who had sufficient experience with the project to ensure these kinds of problems didn't happen.

So I can sympathize with a desire not to go there.  I would say anyone in an OWNERS file needs to be trusted not just to review code that goes into that directory, but to either understand the ramifications on the wider system, or to know what they don't understand and ensure someone more knowledgeable gets looped in in that case.  Perhaps that latter requirement about a trustworthiness towards wider system effects is what you mean be "trusted enough to be in core/OWNERS".  I'm looking for a system that maintains that attribute, it just puts more precise scoping on what parts of the system people understand.

Also, perhaps some directories, like core/layout/, don't make sense to have finer-grained OWNERS files, because they somewhat inherently end up impacting everything, whereas other directories, like core/platform/image-decoders/, are more isolated and thus more suitable for this.  I wouldn't propose myself as a core/layout/ OWNER any more than a core/ OWNER.

Since existing OWNERS approve new ones, hopefully there don't need to be super-strict rules about this, as ideally it's somewhat clear whether someone fulfills these requirements.  I see the coding work you describe as making it easier for everyone, not just OWNERS, to avoid layering violations and subtle interdependency bugs.  To me that doesn't block allowing fine-grained OWNERS today where it makes sense to do so, but if you're more comfortable waiting until then it probably doesn't hurt too much.

PK

[Manuel Rego Casasnovas]

On 17/09/15 00:40, Elliott Sprehn wrote:
> If you're not trusted enough to be in core/OWNERS, you're not trusted
> enough to be in core/layout/OWNERS since you have access to all the same
> API surface there. Just adding people who work on a silo has not worked
> historically, it just means that slio'ed team dumps lots of code into
> their silo touching the rest of the system since it makes reviews easier.

There're 2 people (cbiesinger and svillar) only listed in
core/layout/OWNERS and not in core/OWNERS. And they were added quite
recently [*] so maybe this is not a common policy for everybody.

If we don't want that we should move them to core/OWNERS or remove them
from core/layout/OWNERS.

Just my 2 cents,
Rego

[*] They were added past June:
* Jun 3 2015: https://codereview.chromium.org/1166623007
* Jun 22 2015: https://codereview.chromium.org/1204503002

[Yutaka Hirano]

tyoshino@ and yhirano@ (me) are owners of core/streams and we aren't core owners.

If we are to be removed from core/streams owners, can anyone become core/streams owners instead?

I'm a fan of fine-grained owners and I feel it useful even when it is a subset of core owners.

FYI, here are sub directory owners who aren't core owners.

alanc...@chromium.org

arbe...@chromium.org

bo...@chromium.org

ca...@chromium.org

cbies...@chromium.org

dgo...@chromium.org

al...@chromium.org

le...@chrimium.org (maybe typo of le...@chromium.org who is a core owner).

sh...@chromium.org

svi...@igalia.com

tyos...@chromium.org

yhi...@chromium.org

yo...@chromium.org

[Rick Byers]

I'm also a fan of finer grained core owners.  In past internal discussions among the core owners, I'd got agreement to create core/editing/OWNERS and core/frame/OWNERS (each of which have members who aren't core/OWNERS).

I do share Elliott's concern that the system is too complex and highly coupled to enable isolated review islands.  I'm definitely not comfortable reviewing many core changes, and so regularly add other experts to reviews.  But there's also a lot of low-risk stuff that it's totally worth distributing among a wider set of reviewers who don't have total blink expertise (including myself).  Ultimately I think we have to have a set of engineers who we trust to know what they can confidently review and when they should add others (as we do already for core/OWNERS), and we need some feedback cycle to help people improve in this judgement.  We also need to be OK with allowing engineers to learn from and correct their mistakes.  I know I've certainly benefited from insightful comments Elliott and others have added to a review I'd LGTM'd (possibly even after commit) and adjusted my review habits accordingly.

You could argue that such mistakes should mean I should be removed from core/OWNERS (which frankly would be OK with me if others picked up the slack - less time doing CRs for me).  Personally I'm of the opinion that the damage done by such mistakes is much lower than the value we get from more distributed ownership and team growth opportunities it provides, and so well worth the cost.  Given that, I think the debate is just do we lower our bar for core ownership to admit more area experts, or should we be OK with sub-directory owners who aren't core owners.  Either one is fine with me.

Rick

[Sergio Villar Senin]

I totally agree with Elliott. In my case for example, I have to review many patches for the CSS Grid Layout feature. I'm just OWNER of core/layout and it's a PITA because although most of the code I review belong to layout/ sometimes we need to add a new property (css/ & css/parser) or modify something in the style (style/) and we have to wait for some other OWNER's lgtm to review very small portions of patches that are blocked till that happens.

In any case, being listed in core/OWNERS does not mean that you're going to review anything that passes by, as usual people only review patches related to their area of expertise, but being a sub-directory OWNER and at the same time not being a core/OWNER is quite inconvenient.

BR