Stream: t-release/triage

Topic: Criteria for triage to look at a PR


view this post on Zulip Joshua Nelson (Sep 15 2020 at 00:31):

what are the criteria then? even though it has a force-push it hasn't been reviewed in 16 days

view this post on Zulip DPC (Sep 15 2020 at 00:32):

there was a review already by eddy (even though not the reviewer but "credible" enough - so the reviewer could be likely waiting on those changes to be made

view this post on Zulip Joshua Nelson (Sep 15 2020 at 00:33):

it says waiting-on-review though :/

view this post on Zulip DPC (Sep 15 2020 at 00:34):

yes because it's still waiting on official reviewer, so it's a case of contradictory things being correct :grinning:

view this post on Zulip DPC (Sep 15 2020 at 00:34):

(could have been s-waiting-on-author and wouldn't be wrong)

view this post on Zulip Joshua Nelson (Sep 15 2020 at 00:35):

well, if it had not had changes for 15 days, triage would have taken notice, right? I guess I'm wondering why rebasing to fix merge conflicts should reset the timer

view this post on Zulip DPC (Sep 15 2020 at 00:36):

this wasn't a rebase though, it was changes to match a review, so slightly different

view this post on Zulip Joshua Nelson (Sep 15 2020 at 00:37):

no, that was to address a merge conflict: https://github.com/rust-lang/rust/pull/76043#issuecomment-686853500
you can't see the earlier changes to match a review because github doesn't show them after a force-push

view this post on Zulip DPC (Sep 15 2020 at 00:38):

also generally reviewers wait for the author to resolve conflicts, so it makes sense to reset the timer

view this post on Zulip Joshua Nelson (Sep 15 2020 at 00:38):

it seems like that could lead to really long wait times

view this post on Zulip Joshua Nelson (Sep 15 2020 at 00:39):

imagine a PR that keeps hitting merge conflicts every 10 days

view this post on Zulip DPC (Sep 15 2020 at 00:40):

that has happened tbh, and normally I would re-assign it to speed it once the 2nd iteration of conflicts have been resolved, but this is more from a manual inspection so has to be done on a case-by-case basis

view this post on Zulip DPC (Sep 15 2020 at 00:44):

also the window was moved to 15 days recently, and was 7 days before that (started at 3 days before this group was formed), so chances of a pr constantly hitting conflicts without being picked in a triage window were slim

view this post on Zulip Joshua Nelson (Sep 15 2020 at 00:44):

what was the rationale for increasing that? 15 days without a review seems really frustrating :/

view this post on Zulip DPC (Sep 15 2020 at 00:44):

it does but then reviewers need the bandwidth to review multiple prs

view this post on Zulip DPC (Sep 15 2020 at 00:46):

and given that some people took a break from rust organisation or others got busy because of the changes due to the pandemic, it made sense to widen the window given the surge in prs reviewers have to look into

view this post on Zulip Joshua Nelson (Sep 15 2020 at 00:46):

yeah that's fair

view this post on Zulip DPC (Sep 15 2020 at 00:47):

generally I try to reassign the smaller prs or review them if possible, to share the load and some other members do the same as well :smile:

view this post on Zulip Charles Lew (Sep 15 2020 at 01:16):

@Joshua Nelson I use this filter you can see the conditions. And i clear up the >15 days zone every friday.

view this post on Zulip simulacrum (Sep 15 2020 at 11:35):

FWIW I don't personally find reviewing PRs with merge conflicts bad, but it depends on what those look like - normally they're just "git isn't smart" not actual conflicts, in which case a review doesn't hurt.

view this post on Zulip Charles Lew (Sep 15 2020 at 11:36):

Sometimes i wish i can @rustbot rebase, but i'm not sure how much is that possible.

view this post on Zulip simulacrum (Sep 15 2020 at 11:37):

@DPC are we tracking anywhere something like reviewer latency? I know I try to check everything that's waiting on review for me every few days, but frequently PRs don't get put in my queue and are stuck in some other state

view this post on Zulip simulacrum (Sep 15 2020 at 11:37):

Triagebot can't really help you rebase, it doesn't know how to resolve those conflicts

view this post on Zulip Charles Lew (Sep 15 2020 at 11:42):

Many times small merge conflicts can be resolved by mergetool.

view this post on Zulip Vadim Petrochenkov (Sep 15 2020 at 11:49):

DPC said:

also generally reviewers wait for the author to resolve conflicts, so it makes sense to reset the timer

That's not generally true, and we don't change the label to waiting-on-review on merge conflicts (https://github.com/rust-lang/rust-central-station/pull/34).

view this post on Zulip DPC (Sep 15 2020 at 12:26):

many reviews got delayed because the reviewer was waiting for conflicts to be resolved while the author was waiting for it to be reviewed to resolve the conflicts, thus resulting in a "deadlock". Also the reviewer has to most likely review it again after conflict resolution, so it makes sense to resolve it and then get reviewed and if possible approved straightaway.

view this post on Zulip DPC (Sep 15 2020 at 12:45):

i know some conflicts are small and can be ignored but that involves someone checking them to ascertain if a conflict applies to a pr or not

view this post on Zulip Joshua Nelson (Sep 15 2020 at 12:46):

I think if we're having debate over it, it should be a case-by-case basis. So triage should assume the reviewer isn't waiting for a rebase, and the reviewer should comment if they are.

view this post on Zulip DPC (Sep 15 2020 at 12:48):

Joshua Nelson said:

I think if we're having debate over it, it should be a case-by-case basis. So triage should assume the reviewer isn't waiting for a rebase, and the reviewer should comment if they are.

that would depend on the type of conflict

view this post on Zulip Joshua Nelson (Oct 23 2020 at 19:07):

DPC said:

Joshua Nelson said:

I think if we're having debate over it, it should be a case-by-case basis. So triage should assume the reviewer isn't waiting for a rebase, and the reviewer should comment if they are.

that would depend on the type of conflict

that's kind of my point though - right now we do that for all conflicts, even minor ones that don't make it harder for the reviewer


Last updated: Jan 29 2022 at 09:01 UTC