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

Homu's queue should be ordered by insertion time, not PR age #37107

Closed
nnethercote opened this issue Oct 12, 2016 · 3 comments
Closed

Homu's queue should be ordered by insertion time, not PR age #37107

nnethercote opened this issue Oct 12, 2016 · 3 comments
Labels
T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@nnethercote
Copy link
Contributor

My PR at #37064 received r+ 56 hours ago. It entered Homu's queue at #5, and made it as high as #3 not long after that. It's now #14 in the queue. It got added to a roll-up, but that roll-up failed tests, and so I have no idea when it will be merged.

I find this very, very frustrating. When a PR of mine is finished I like for it to be merged ASAP, to minimize the chance of merge conflicts, and so I can forget about it and focus entirely on other things.

If Homu simply merged PRs in the order that they were added to the list, that would make things fairer and more predictable. (Having the ability to bump priorities for things like roll-ups would still be fine; some merges have good reason to be higher priority.)

I anticipate some counter-arguments, so let me preemptively rebut them.

  • "It'll get merged eventually, and the merge order doesn't matter." To a neutral observer of the entire system, correct, it doesn't much matter. To an individual developer, it definitely does matter. (Trust me, I am deeply frustrated about Avoid allocations in Decoder::read_str. #37064's progress right now!)
  • "The real problem is that merging is too slow." Well, yes, if merging took no time then this would matter a lot less. But that's not the case, and making it not the case sounds difficult and not likely to happen soon. Whereas changing the queue ordering sounds straightforward, and so would be easy to do quickly.
  • "If the queue gets long your PR will end up in a roll-up anyway." Perhaps, but if it gets bumped repeatedly before going into the roll-up, it will end up taking a lot longer to get merged than it would have with a fair algorithm.

Fairness and predictability, that's all I want! :)

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 12, 2016

When a PR of mine is finished I like for it to be merged ASAP

Ha-ha!

Homu's queue should be ordered by insertion time

I think, the first time the PR is r+'d (approved) should be used as a metric, so spurious failures and rebases (which happen often) don't move PRs to the end of the queue.
Or even more precise, first r+ not followed by r-, i.e. r- returns the PR into "unapproved"/"in review" state.

@brson
Copy link
Contributor

brson commented Oct 13, 2016

I think sorting by order of bors approval, where each r+ or retry adds the PR to the back of the list, would probably be the most unsurprising, and the best experience for casual contributors. It does punish retries, but that's an issue for reviewers and maintainers, and better to punish them than casual contributors.

@Mark-Simulacrum Mark-Simulacrum added T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. and removed T-tools labels May 14, 2017
@Mark-Simulacrum
Copy link
Member

We discussed this at the infra team meeting yesterday, and the current belief is that while this is something we might want, most people felt either ambivalent or could see both sides of the argument. As such, I'm going to close this issue. If you'd like to reopen, please do so here: https://github.com/rust-lang/rust-central-station.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants