# Dealing with PR latency

## "Ideal" world

As PRs are opened, they get reviewed quickly enough that:

1) Other work is not blocked
2) Authors do not "move on"
* If the author is "new" and trying to learn, this may be "quick", so they don't mentally lose track of what they did
* If the author is "experienced" (member of the project or regular contributor), this may be "less quick"
3) PRs do not sit so long that their status becomes unclear
* It's easy for PRs to bitrot, where they now are both sort of "waiting on review" and "waiting on author" (to rebase)

PRs that are not "waiting for review" should also be moving forward *somehow*
- "bors", ACP", "fcp", "perf", "crater" -> automatic
- "bikeshed" -> remove in favor of "waiting for team"?
- "team" -> Some defined question to be answered by the relevant team in some meeting
- Meeting can be dedicated (like a design meeting), or incidental (like a more general triage meeting)
- Meeting might not be "planned" (i.e. the meeting will happen at "some point", like some to-be-scheduled triage meeting)
- "author" -> Some defined task for author to do
- Examples: rebasing, responding to review, more work
- "blocked" -> Some defined *and actionable* precursor
- Good example: another PR needs to land
- Bad example: we need a new trait solver
- "experimental" -> More lenient, but ultimately should be "active", mostly defined by the author
- Author may want to use CI resources or non-formal review or thoughts
- Generally low maintenance cost (don't necessarily need to be triaged)

## Key concerns

- We don't have great metrics for review lag, other than coarse, static data.
- Has the time from open to close/merge gotten longer over time?
- Are reviewers "at capacity" (for time or mentally) already?
- At some point, PRs will come in faster than can be reviewed - are we there already
- What can we do to reduce PR review burden?
- Automation (??)
- Better review assignment (more granular by team, dedicated review teams for speedy review, etc.)
- If we *do* reach capacity, what do we do?
- Continue to assign reviews to reviewers at capacity?
- Stop assigning PRs immediately (and instead wait for capacity)?
- Increase capacity? (more reviewers)
- When reviewers want to take breaks from reviewing (vacations, burn out, too busy), what's the best way to do that?
- Currently, it's only reviewing or not.
- Would it be helpful to have easier ways to slow (temporarily or permanently) or stop reviews (temporarily)

## Data

Methods of gathering data:
- Github web ui sleuthing
- https://rust-lang.github.io/rustc-pr-tracking/
- https://github.com/jackh726/rust-pr-explorer
- Some screenshots/discussion here: https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/PR.20tracking
- `t-release/triage` zulip history

Current data from github:
- 693 open
- 342 open tagged `waiting-on-review`
- 116 open tagged `waiting-on-review`, `T-compiler`
- 322 open tagged `T-compiler`

From rustc PR tracking:
- One year ago, 536 PRs open
- review: 266 -> 340
- author: 174 -> 241 (actually grew more)
- Last activity at more than a month is largest increase

999 days:


### Some observations from triage

- checking PRs progress is currently mostly manual, takes 2 to 3 hrs/week
- current (informal) policy is to ping the reviewer after about 3w on `S-waiting-on-review`
- a bulk of PRs visited every week are consistently blocked (for reasons)
- sometimes PR authors forget - or don't know - that a PR will not get in the review queue unless the review flag is promptly switched
- a few PRs are stuck after profiling (fixable with some automation - see proposal 3)
- rarely PRs need to have the reviewer rerolled (the PR will progress, *eventually*)

## Proposals and ideas

### 1) Form a subteam of people able to commit more to reviewing and automatically reroll a reviewer when a PR seems stuck

* [HackMD document](https://hackmd.io/UzP5JacHRuWo-X-QfzdFSQ)
* [discussion on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Reducing.20PR.20review.20time)

PRO: Some reviewers are {full,part}time paid and could agree to be on this subteam
CONS: Does not really solve the underlying problem (i.e. we need more people). Adds a bit of organization overhead? Discussion on Zulip didn't receive a lot of feedback.

### 2) Setting oneself off from the review rotation for a period of time

* [HackMD document](https://hackmd.io/GqXA4nyTQ4m__hRZUayVRg)
* [Zulip discussion](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/review.20latency.20meeting.20planning/near/327884080)

PRO: That would allow skipping people away from reviews
CONS: Some edge cases discussed on Zulip

### 3) Automatic switch of the review flag to `S-waiting-on-author` after a PR has been profiled

* [Zulip discussion](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202023-02-09/near/326851255)

No objections so far, seems reasonable.

### Discussed questions/comments

#### re "sometimes PR authors forget - or don’t know - that a PR will not get in the review queue unless the review flag is promptly switched"

Seems like we should perhaps update rustbot/triagebot to tell first time contributors about the status labels. Right now it doesn't look like there are any special messages for first timers and the contributing guide linked doesn't mention the status labels either except in one small sentance.

Jack: yes, let's do that. Anyone up to make a PR for that to triagebot?

#### "people on the review rotation who don't regularly review" seems like a very bad state of affairs

"It's okay for team members not to be on the rotation, but not for them to get assigned to PR's that they let sit for months"

"I'm aware of people who never sit down to look at PRs they're assigned to, and they only get looked at if the reviewer is pinged on zulip"

Jack: Let's brainstorm and implement reassigning PRs from overloaded reviewers async

#### re "What can we do to reduce PR review burden?"

What if there was a pre-review queue for not-yet-contributors-but-active people such as Vincenzo Palazzo or myself (Bryan Garza) to leave more basic feedback before/whilst the review gets looked at by a contributor/member with more subject-matter knowledge? Could help reduce the amount of work for reviewers if they can +1 some of the existing comments on the PR and leave final feedback.

Jyn: Newcomers may have trouble rebasing and leave
Jyn: I would love for bors to link to https://rustc-dev-guide.rust-lang.org/git.html#rebasing-and-conflicts as soon as a PR gets conflicts

Review steal subteam:
- Put together and evaluate in ~6 months
Should also make sure existing policy of encouraging initial reviews by others is written somewhere

#### 3) Automatic switch of the review flag to S-waiting-on-author after a PR has been profiled

* [Zulip discussion](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202023-02-09/near/326851255)

@jynelson expressed doubts. What do the others think?
pnkfelix: let's discuss this async, time is up

### Questions/comments

#### How do I ask a question

Like this :)

#### re. "increasing review capacity": Are there team members that "should" be reviewing that aren't?

If so, how do we encourage them to? Should there ever be some "mandatory" review queue requirement for team members?

#### what is better: leaving PR's unassigned entirely, or potential review overload?

pnkfelix: There are times when, for *issues*, I have assigned things to myself during triage becauase I say "well, if no one else is taking this, then I guess I will." And then it doesn't always get addressed. I do try to avoid that behavior when it comes to PR's, but maybe I've made that mistake there, I'm honestly not sure. In any case, the question I have is: We do strive to ensure that P-critical and P-high *issues* always have an owner assigned. We do not currently have any notion of prioritization to the *PRs* though. And instead they just get assigned automatically.

#### PRs do not sit so long that their status becomes unclear

bryangarza: Maybe rustbot could leave some additional information about the reviewer it has picked, namely, at what cadence do they usually review (roughly) -- some reviewers look daily, others batch their reviews weekly etc. It would help a PR author to get a better idea of when it might be looked at. Though I wonder if revealing this info might make the PR author want to reroll to try to get someone that reviews more frequently...

Seems that long-time contributors already know how often each reviewer reviews, but they just gained this knowledge over time as they submitted more PRs. Or they know, "I might have to ping this person so they will take a look".