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

Initial aggregator picks. #13974

Merged
merged 14 commits into from
May 19, 2017
Merged

Initial aggregator picks. #13974

merged 14 commits into from
May 19, 2017

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented May 1, 2017

Fixes #13558

Initial picks for wiring the aggregator in from the API server for alpha enablement.

[test]

@deads2k deads2k force-pushed the agg-02-pick branch 2 times, most recently from af5043d to 0de5083 Compare May 1, 2017 17:05
@deads2k
Copy link
Contributor Author

deads2k commented May 2, 2017

@ncdc looks like it was just a networking test flake. Lots of picks, two small commits on top for fitting

@ncdc
Copy link
Contributor

ncdc commented May 3, 2017

[test]

@ncdc
Copy link
Contributor

ncdc commented May 3, 2017

LGTM [merge]

FYI @liggitt @smarterclayton @sttts in case you want to review too.

@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label May 15, 2017
@deads2k
Copy link
Contributor Author

deads2k commented May 17, 2017

@stevekuznetsov looks like the queue prioritization may be broken. I'm seeing very recent pulls ahead of this one on https://ci.openshift.redhat.com/merge_queue/origin.html

@ncdc do you want to try to stack even deeper on this pull (it's already got 14 commits) to try to make it in time for dcut?

@bparees
Copy link
Contributor

bparees commented May 17, 2017

@stevekuznetsov looks like the queue prioritization may be broken. I'm seeing very recent pulls ahead of this one on https://ci.openshift.redhat.com/merge_queue/origin.html

pulls are ordered by the commit date, not the PR open date.

@deads2k
Copy link
Contributor Author

deads2k commented May 17, 2017

pulls are ordered by the commit date, not the PR open date.

No they aren't. #14177 from five days ago has this commit e4f315b also five days ago

@deads2k
Copy link
Contributor Author

deads2k commented May 17, 2017

Not that I'm whining about a typo fix taking 90 minutes in our queue or anything... Wish we had retest-not-required

@bparees
Copy link
Contributor

bparees commented May 18, 2017

No they aren't. #14177 from five days ago has this commit e4f315b also five days ago

yeah, they are.

from #14177:

commit e4f315bc676381f628fa0dfde6962dbef520b0bc
Author:     John Matthews <[email protected]>
AuthorDate: Fri May 12 14:36:40 2017 -0400
Commit:     John Matthews <[email protected]>
CommitDate: Fri May 12 14:36:40 2017 -0400

    Typo fix of 'execure' on oc cluster up --help

from your pr:

commit 56555fd3609fb06601a08a3962bb87faa2240957
Author:     deads2k <[email protected]>
AuthorDate: Mon May 1 09:14:01 2017 -0400
Commit:     deads2k <[email protected]>
CommitDate: Mon May 15 10:18:31 2017 -0400

his commit date is may 12th, yours is may 15th.

@deads2k
Copy link
Contributor Author

deads2k commented May 18, 2017

his commit date is may 12th, yours is may 15th.

Heh. That means people who have to rebase constantly lose position. Ouch.

@bparees
Copy link
Contributor

bparees commented May 18, 2017

Heh. That means people who have to rebase constantly lose position. Ouch.

yup. it also means one jerk w/ a broken PR and an old commit date can hog the top of the queue indefinitely by hammering the merge tag.

@deads2k
Copy link
Contributor Author

deads2k commented May 18, 2017

yup. it also means one jerk w/ a broken PR and an old commit date can hog the top of the queue indefinitely by hammering the merge tag.

Really hoping that wasn't me.

@bparees
Copy link
Contributor

bparees commented May 18, 2017

Really hoping that wasn't me.

it's only a theoretical exploit. not aware of anyone who's done it.

@deads2k
Copy link
Contributor Author

deads2k commented May 18, 2017

re[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 56555fd

@deads2k
Copy link
Contributor Author

deads2k commented May 18, 2017

re[merge] failed on idling before.

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1559/) (Base Commit: eb0b1b8)

@deads2k
Copy link
Contributor Author

deads2k commented May 18, 2017

#14236 re[merge]

@deads2k
Copy link
Contributor Author

deads2k commented May 18, 2017

#14236 re[merge]

@deads2k
Copy link
Contributor Author

deads2k commented May 18, 2017

it's only a theoretical exploit. not aware of anyone who's done it.

Yep, that works. Green test though. :)

@deads2k
Copy link
Contributor Author

deads2k commented May 18, 2017

#14236 re[merge]

1 similar comment
@deads2k
Copy link
Contributor Author

deads2k commented May 19, 2017

#14236 re[merge]

@stevekuznetsov
Copy link
Contributor

The argument for it working this way was that it rewards people who finish their code, instead of penalizing people who couldn't get their code reviewed. As faking the commit date is easy, if it is found that developers are using that to game the queue, we will need to reconsider how pull requests are ordered.

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 56555fd

@openshift-bot
Copy link
Contributor

openshift-bot commented May 19, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/698/) (Base Commit: ab8f4fc) (Image: devenv-rhel7_6240)

@openshift-bot openshift-bot merged commit b2d7377 into openshift:master May 19, 2017
@deads2k
Copy link
Contributor Author

deads2k commented May 19, 2017

The argument for it working this way was that it rewards people who finish their code, instead of penalizing people who couldn't get their code reviewed. As faking the commit date is easy, if it is found that developers are using that to game the queue, we will need to reconsider how pull requests are ordered.

The end effect is that structural change becomes virtually impossible since every rebase pushes you back to the end of a 24 hour+ queue. Not being familiar with the code, is it easy to order on first [merge] tag? That has worked well upstream.

@stevekuznetsov
Copy link
Contributor

That has worked well upstream.

When I proposed that approach in the past I was met with resistance from members of the team who thought the current approach was ideologically better. We can revisit the subject if you'd like - start a thread on the mailing list, maybe? Unclear what the general attitude is today.

@deads2k deads2k deleted the agg-02-pick branch August 3, 2017 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants