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

[HAMMER] Turbo distinct #19137

Merged
merged 2 commits into from
Aug 15, 2019
Merged

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Aug 12, 2019

Turbo distinct

(cherry picked from commit ec3a2dc)

Manual backport of #19025 to hammer to avoid a git conflict.
https://bugzilla.redhat.com/show_bug.cgi?id=1741327

@miq-bot miq-bot changed the title Merge pull request #19025 from kbrock/turbo_distinct [HAMMER] Merge pull request #19025 from kbrock/turbo_distinct Aug 12, 2019
@jrafanie jrafanie changed the title [HAMMER] Merge pull request #19025 from kbrock/turbo_distinct [HAMMER] Manual backport of: Merge pull request #19025 from kbrock/turbo_distinct Aug 12, 2019
@jrafanie jrafanie mentioned this pull request Aug 12, 2019
@jrafanie jrafanie removed the blocker label Aug 12, 2019
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@simaishi simaishi changed the title [HAMMER] Manual backport of: Merge pull request #19025 from kbrock/turbo_distinct [HAMMER] Turbo distinct Aug 14, 2019
@simaishi
Copy link
Contributor

@jrafanie Created clone BZ https://bugzilla.redhat.com/show_bug.cgi?id=1741327, can you please update commit msg so bot will comment on the clone BZ instead?

@simaishi
Copy link
Contributor

@jrafanie Can you review the Travis failure?

@jrafanie
Copy link
Member Author

jrafanie commented Aug 15, 2019

@simaishi Ok, I fixed the sporadic failure on this PR. I'll have to open a PR on ivanchuk to pick that commit..it will sporadically fail there and on master.

It's actually not failing on ivanchuk or master, so this is hammer only.

@miq-bot
Copy link
Member

miq-bot commented Aug 15, 2019

Checked commits jrafanie/manageiq@14ea397~...f3201ee with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

lib/rbac/filterer.rb

@jrafanie
Copy link
Member Author

@simaishi it's green now

@simaishi simaishi merged commit af0938e into ManageIQ:hammer Aug 15, 2019
@simaishi simaishi added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 15, 2019
@simaishi
Copy link
Contributor

Travis was fine yesterday but it's failing today with:

  1) Rbac::Filterer common setup with inline view remembers distinct
     Failure/Error: expect(recs.map(&:id)).to eq(vms.sort_by(&:updated_on).reverse.map(&:id))

       expected: [75000000002599, 75000000002598, 75000000002597]
            got: [75000000002597, 75000000002598, 75000000002599]

Failed before with:

       expected: [18000000000277, 18000000000278, 18000000000279]
            got: [18000000000279, 18000000000278, 18000000000277]

So it looks like the order isn't guaranteed.

@jrafanie
Copy link
Member Author

@simaishi is this still an issue? cc @kbrock

@simaishi
Copy link
Contributor

@jrafanie yes, that's still an issue. Sometimes it fails, other times it works...

@kbrock
Copy link
Member

kbrock commented Aug 31, 2019

@jrafanie @simaishi Does this ever fail for other branches?

am looking into this

@simaishi
Copy link
Contributor

I've seen this fail in hammer branch only so far.

@kbrock
Copy link
Member

kbrock commented Sep 5, 2019

running this test locally, I was able to get it to fail 1 of 100 times.
after backporting #18928 it fails for me 100/100 times
after reverting this PR, it succeeds for me 100/100 times

@jrafanie ok to revert this PR?

@jrafanie
Copy link
Member Author

jrafanie commented Sep 5, 2019

running this test locally, I was able to get it to fail 1 of 100 times.
after backporting #18928 it fails for me 100/100 times
after reverting this PR, it succeeds for me 100/100 times

Ah, are you saying that #18298 + the first commit in this PR "fix" the problem?

@jrafanie ok to revert this PR?

I'm guessing you mean revert the second commit of this PR, f3201ee, ... if yes, then, yes, please open a hammer PR to revert f3201ee

@kbrock
Copy link
Member

kbrock commented Sep 5, 2019

@jrafanie thnx - yes, just revert f3201ee

@jrafanie jrafanie deleted the turbo_distinct_hammer branch July 15, 2022 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants