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

Jump to bottom

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

Closed
mdempsky opened this issue Mar 8, 2019 · 7 comments
Closed

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

mdempsky opened this issue Mar 8, 2019 · 7 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@mdempsky
Copy link
Member

mdempsky 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
Copy link
Contributor

agnivade commented Mar 9, 2019

/cc @andybons @dmitshur

@bcmills
Copy link
Contributor

bcmills 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.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 9, 2019
@gopherbot
Copy link

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

@bcmills
Copy link
Contributor

bcmills commented Apr 9, 2019 •
edited

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
Copy link
Contributor

bcmills 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 x/build issues (builders, bots, dashboards) label Apr 15, 2019
@gopherbot
Copy link

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

@gopherbot
Copy link

Change https://go.dev/cl/443061 mentions this issue: cmd/gopherbot: limit number of reviewers assigned

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 17, 2022
@golang golang locked and limited conversation to collaborators Oct 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@mdempsky @agnivade @dmitshur @bcmills @gopherbot