Stream: t-compiler

Topic: "Review latency can be challenging"


Vadim Petrochenkov (May 15 2020 at 16:12, on Zulip):

The compiler team survey (https://hackmd.io/E0i2Re5vQy6iUSPhdo2IiA?view#Summary) includes this note, and I thinks it's important.
With alexcrichton, michaelwoerister, Zoxc and Centril all stopping reviewing in a short period of time the number people who do regular reviews of compiler PRs became smaller, so it became even more important.

Can we encourage something like Google reviewing guide https://google.github.io/eng-practices/review/reviewer/?
https://google.github.io/eng-practices/review/reviewer/speed.html in particular.

The main thing is "prioritize reviewing over your own individual work" basically.
Reviews unblock further work for multiple people rather than just you, unless you are doing something really infrastructural, so prioritizing it increases our throughput as a team.
The last year I tried to schedule some reviewing time, even it's small, every day and can confirm that it does work like this from my experience.

Realistically, the time until review can be 1-2 days for small PRs and 1 week for large PRs, if we need specific goals.
I could write a bit about how I organize this process, but one point is that allocating only 10 minutes a day to triage the review queue, set status labels, r+ trivial PRs, and respond to trivial questions/CI failures already makes things move better.

One unpleasant aspect of this story is that some people became known as slow reviewers and PRs assigned to them are reassigned, because without reassigning you are never going to do anything productively if it requires multiple PRs.
The PRs are reassigned to more known to be active reviewers, so the reviewing load become skewed.

nikomatsakis (May 15 2020 at 16:18, on Zulip):

Yeah, I've been thinking a lot about this lately.

nikomatsakis (May 15 2020 at 16:18, on Zulip):

I try to assign time for reviewing every day

nikomatsakis (May 15 2020 at 16:18, on Zulip):

but it is challenging, particularly when you add in keeping up with notifications

nikomatsakis (May 15 2020 at 16:24, on Zulip):

I wonder if one thing we could do is to kind of gamify a bit -- i.e., celebrating reviewers more, and also that would help to track how the load is both being distributed but also being satisfied

nikomatsakis (May 15 2020 at 16:24, on Zulip):

we had some graphs for a time viewing PR latency and the like

pnkfelix (May 15 2020 at 17:42, on Zulip):

I don't doubt that I'm someone known as a "slow reviewer", at best

pnkfelix (May 15 2020 at 17:43, on Zulip):

@Vadim Petrochenkov 's note is indeed important: The idea that a PR awaiting review is indeed higher priority than almost anything a reviewer is likely to have on their plate is an important idea, and one that I don't think I've heard stated quite that way before.

Jake Goulding (May 15 2020 at 18:08, on Zulip):

Pointing out what is hopefully obvious, but reviewing isn't as fun as writing code.

mark-i-m (May 15 2020 at 18:14, on Zulip):

Writing docs is also not as fun as writing code for most people :) That's why it might make sense to have more people paid to do this sort of work...

mark-i-m (May 15 2020 at 18:14, on Zulip):

(fwiw, I am one of those oddball people who enjoy writing docs)

lcnr (May 15 2020 at 18:28, on Zulip):

I currently feel somewhat review bound, so if there is a good way for a non "contributor" to help out here I would gladly do so .

Wesley Wiser (May 15 2020 at 18:33, on Zulip):

Sometimes I've thought about picking up a review and leaving feedback but if I don't feel comfortable r+ing then I feel like I'm not really helping because someone else still has to review and now it looks like I'm going to r+ it since I left a review.

Wesley Wiser (May 15 2020 at 18:33, on Zulip):

I'm not sure what the review etiquette is

oli (May 15 2020 at 18:36, on Zulip):

Wesley Wiser said:

Sometimes I've thought about picking up a review and leaving feedback but if I don't feel comfortable r+ing then I feel like I'm not really helping because someone else still has to review and now it looks like I'm going to r+ it since I left a review.

I've brought this up before, because I was in the same situation. I just remember that everyone was in favour of having review work being taken away from them. It hasn't always worked out perfectly in practice, but never with an issue that couldn't be remedied by an r- or a follow up

Wesley Wiser (May 15 2020 at 18:56, on Zulip):

Thanks, that's good to know.

davidtwco (May 15 2020 at 19:15, on Zulip):

I’m open to something like this.

(currently my reviews are slower than usual and I’ve been reassigning more because I’ve not got much time with exams - they end in four days though - so apologies to anyone who’s felt the effects of that)

nikomatsakis (May 15 2020 at 22:14, on Zulip):

I strongly favor people leaving reviews without a final r+

nikomatsakis (May 15 2020 at 22:14, on Zulip):

it's very helpful to me to know somebody else already looked it over

nikomatsakis (May 15 2020 at 22:14, on Zulip):

so I have to be a bit less picky

nikomatsakis (May 15 2020 at 22:14, on Zulip):

it's particularly useful if they call attention to things they're not sure about

nikomatsakis (May 15 2020 at 22:15, on Zulip):

or sort of leave comments on the "key areas" of the PR

nikomatsakis (May 15 2020 at 22:15, on Zulip):

plus I feel like 99% of my reviews the first round is just me saying "document this, document that"

XAMPPRocky (May 16 2020 at 08:09, on Zulip):

Along these lines I've thought about what if there was a way for you as a member to communicate your bandwidth to triagebot/highfive, by for example giving a limit on how many S-waiting-on-review PRs can be assigned to you. Then once the all members of a team's queues fill up there's a new status like S-waiting-on-reviewer that's essentially the team wide backlog.

Félix Fischer (May 16 2020 at 08:43, on Zulip):

@Jake Goulding I'm the kind of oddball that enjoys doing review work :3
I find the discussion and the back and forth on the PR's topic really satisfying. Also I get to give kudos, which is something I tend to enjoy as well. There are many good practices that go unnoticed, and when I do notice them I like to give some :heart:️s to the coder that followed them :smile:

Félix Fischer (May 16 2020 at 08:46, on Zulip):

I think that @XAMPPRocky's idea could help a lot. Saying beforehand what is exactly your bandwidth is a way of establishing some healthy boundaries, which is never bad

Félix Fischer (May 16 2020 at 08:47, on Zulip):

It also ensures that you will tend to have time to review the PRs assigned to you, on a timely manner.

oli (May 16 2020 at 10:41, on Zulip):

nikomatsakis said:

it's very helpful to me to know somebody else already looked it over

Maybe we should add a tag to PRs that haven't gotten a review in a week that contributors can look for to get into reviewing without any pressure for completeness or anything. Basically how E-mentor is "you implement something, but someone else is in charge of the big picture" for issues, but for reviewing. You review, but you know that your review only helps move things forward and does not put any responsibility on you.

Charles Lew (May 16 2020 at 10:49, on Zulip):

t-release/wg-triage is actually producing reminders on these regularly (several times a week,) the list is currently produced and posted on Discord wg-triage channel. It includes PRs for all teams (including t-libs and t-rustdoc and so on) though.

Jake Goulding (May 16 2020 at 10:55, on Zulip):

Also I get to give kudos, which is something I tend to enjoy as well. There are many good practices that go unnoticed, and when I do notice them I like to give some :heart:️s to the coder that followed them :smile:

This is one of the most powerful things you can do during a review, @Félix Fischer and it’s something that I’m terrible at. It’s heartening to know that other people actually do it.

RalfJ (May 16 2020 at 11:00, on Zulip):

I know I've sometimes been slow to review stuff. That's mostly because when I use my free time in the evening/weekend to do Rust stuff, I regularly feel much more like doing "fun stuff" (like writing code/docs myself) rather than reviewing. If I would have more time for Rust I could do both (coding and reviewing), but that's a challenge to do in just my spare time.

RalfJ (May 16 2020 at 11:02, on Zulip):

Some of these were significant changes in the core of Miri so I feel I'd have to review them anyway even if someone else r+'d... not entirely sure what to do about that.

RalfJ (May 16 2020 at 11:02, on Zulip):

I'm very happy I dont get auto-assigned to PRs.^^

Jake Goulding (May 16 2020 at 11:03, on Zulip):

I strongly favor people leaving reviews without a final r+

This has come up in my $DAYJOB recently; it’s been semi-contentious.

The discussion has centered around who is responsible for seeing that the drive-by reviews are implemented. Does the assigned person have to check that every arbitrary comment was followed, even if they didn’t leave it?

There’s also the problem around what to do if multiple reviewers disagree on a point.

Jake Goulding (May 16 2020 at 11:04, on Zulip):

Another problem that I have experienced is around the strength of a suggestion. When reviewing, I might have a passing comment that means “oh it might be better to do this” and another comment that means “this must be done before this can be merged” but I am not always clear on directly stating that on every given point.

Jake Goulding (May 16 2020 at 11:07, on Zulip):

Another thing to keep in mind is that a contributor could feel overwhelmed if a large number of people were to review a pull request and comment on all the different things they see. It might feel like trying to defend your dissertation (I think; I have not experienced a dissertation myself :joy:)

RalfJ (May 16 2020 at 11:08, on Zulip):

Jake Goulding said:

Another thing to keep in mind is that a contributor could feel overwhelmed if a large number of people were to review a pull request and comment on all the different things they see.

even more so if the reviewers start arguing because they disagree on something.^^
OTOH, such discussions can be important and the PR is clearly the right place for them IMO.

Jake Goulding (May 16 2020 at 11:09, on Zulip):

And then add on to all of this the complexity that not every given human responds exactly the same way to each situation, so everything we say may or may not apply to any given person. Human interaction can be exhausting :sunglasses:

RalfJ (May 16 2020 at 11:15, on Zulip):

man, people. technology would be so much easier without them. :P

Josh Triplett (May 16 2020 at 15:57, on Zulip):

@XAMPPRocky That sounds like a great idea. Perhaps something to report as a wishlist issue on highfive?

XAMPPRocky (May 16 2020 at 15:59, on Zulip):

Well I think it’d be triagebot at this point.

XAMPPRocky (May 16 2020 at 16:02, on Zulip):

But yes, I can do that

Josh Triplett (May 16 2020 at 16:02, on Zulip):

Oh? What is the difference between the two?

simulacrum (May 16 2020 at 16:06, on Zulip):

highfive is the older tooling which is largely unmaintained, but triagebot is the replacement (and expansion in capability)

mark-i-m (May 16 2020 at 21:08, on Zulip):

RalfJ said:

man, people. technology would be so much easier without them. :P

Yeah, especially users! They're always demanding little nit-picky things like readable fonts and correctness :(

XAMPPRocky (May 19 2020 at 14:19, on Zulip):

Took me a bit but I've filed triagebot#558 around auto-assignment.

DPC (May 20 2020 at 01:10, on Zulip):

Just came across this thread. It's why we (triage-wg) have gone lax with the pinging strategy and we allow prs to stay unreviewed for a longer period so that reviewers get more time to work on the PRs. I'll go through this discussion again to see ways we can improve it :slight_smile:

nikomatsakis (May 20 2020 at 15:33, on Zulip):

Jake Goulding said:

This has come up in my $DAYJOB recently; it’s been semi-contentious.

Interesting points. I think maybe it would make sense to create some "guidelines" for writing "auxiliary reviews". I still think it's a net-win, but I would probably favor some rules like

I think one thing that is really great (and rarely contentious) is to focus on the tests -- i.e., looking and thinking about things that should be tested is a really useful kind of review.

Anyway, I still think that "drive-by reviews" are good, and I suspect most PR authors are happy to get some prompt interaction versus silence (but there is a time when there's too much)

Félix Fischer (May 21 2020 at 02:48, on Zulip):

I think one thing that is really great (and rarely contentious) is to focus on the tests -- i.e., looking and thinking about things that should be tested is a really useful kind of review.

I agree! I find ideas for tests really useful when somebody's reviewing a PR of mine. It helps me understand the entire thing (the problem, the solution, and everything in between) a little bit better.

RalfJ (May 21 2020 at 08:36, on Zulip):

XAMPPRocky said:

Took me a bit but I've filed triagebot#558 around auto-assignment.

It would be great if we could not implement auto-assignment in triagebot, but also make some improvements to the auto-assignment system.

did you mean "if we could not only implement auto-assignment"?
the current text there confuses me.

XAMPPRocky (May 21 2020 at 10:32, on Zulip):

Yes, that’s what I meant, sorry

Last update: Jun 04 2020 at 18:40UTC