Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/build/cmd/gopherbot: suggest multiple reviewers, but only add one #30695

Open
mdempsky opened this issue Mar 8, 2019 · 6 comments

Comments

@mdempsky
Copy link
Member

commented Mar 8, 2019

Gopherbot seems to add a lot of reviewers and CCs to some CLs.

For example, take the current patch series to add aix/ppc64 support. This patch series includes many trivial changes like https://go-review.googlesource.com/c/go/+/164014/3 (1-line change to a test file), but 4 reviewers were added, and another 4 CC'd. Many other CLs have an equally large number of reviewers/CCs.

I was added to a bunch of the CLs, but I've basically been ignoring them since I saw @ianlancetaylor and @bcmills were reviewing some of them. Maybe I should take a closer look at some, but the false positive rate is high enough that I'd probably miss if they specifically asked me for help.

Basically, I'm worried that this is leading to the bystander effect, where Gopherbot is adding reviewers to ensure new contributors' CLs don't go ignored. But then all of the reviewers assume another reviewer will look at it, thereby yielding the same result.

It also leads to situations where I upload a CL for trybot testing, and it gets mailed out to a dozen reviewers, and I have to then remove them all.

--

As a concrete alternative, I suggest gopherbot pick only one of the reviewers to automatically add to the CL, but that it includes in a comment other possible reviewers. I expect this would mitigate the bystander effect, increase the signal-to-noise ratio of review request emails, and still provide an easy escalation path for CL authors when a reviewer isn't responsive.

@gopherbot gopherbot added this to the Unreleased milestone Mar 8, 2019

@agnivade

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

So, why did GopherBot add so many reviewers in the first place?

If they're all listed as owners of the same component... well, maybe we need to prune down the owners or split out subcomponents.

If the CL touches lots of components, then GopherBot should pick the most important one or two, or try to find an owner in the intersection.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 9, 2019

Change https://golang.org/cl/171238 mentions this issue: cmd/gopherbot: assigned reviewers based on the path in the commit message

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

I sent a CL to address gopherbot's over-aggressiveness in general, but note that it wouldn't have helped for the particular example here.

There are eight(!) owners listed in dev.golang.org/owners for go/src/runtime: four primaries (@aclements, @rsc, @RLH, and @randall77), and four secondaries, and no obvious way for a contributor (casual or otherwise) or a bot to determine which of those owners to choose as the primary reviewer review. (Note that gopherbot already introduces an intentional delay before assigning reviewers: if you know who should review the CL, you should add them preemptively when you upload it.)

In general, I think we should probably have no more than two primary owners for a given component, and those owners can choose to route the incoming reviews however they see fit (by adding or removing reviewers as necessary).

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

(Also note that the os and sync packages follow a similar pattern of over-ownership. Perhaps we should add some way to annotate owners by GOOS and/or GOARCH?)

@bcmills bcmills changed the title x/tools/cmd/gopherbot: suggest multiple reviewers, but only add one x/build/cmd/gopherbot: suggest multiple reviewers, but only add one Apr 15, 2019

@gopherbot gopherbot added the Builders label Apr 15, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Apr 16, 2019

Change https://golang.org/cl/170863 mentions this issue: cmd/gopherbot: CC triaged issues to owners

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.