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

Fix "query in loop" in InfraConversionThrottler #18865

Closed
ghost opened this issue Jun 13, 2019 · 8 comments
Closed

Fix "query in loop" in InfraConversionThrottler #18865

ghost opened this issue Jun 13, 2019 · 8 comments

Comments

@ghost
Copy link

ghost commented Jun 13, 2019

When implementing #18860, @Fryguy noticed that https://github.com/ManageIQ/manageiq/blob/master/lib/infra_conversion_throttler.rb#L4 feels like a terrible N+1 (query in a loop is bad).

This issue track the effort to get rid of this bad thing.

@Fryguy
Copy link
Member

Fryguy commented Jun 13, 2019

That entire method is N+1s nested in other N+1s. You have an outer loop which has multiple inner loops and those inner loops have more loops inside there, with individualized queries inside. I recommend doing a complete performance check on it.

cc @kbrock

@ghost
Copy link
Author

ghost commented Jun 13, 2019

@miq-bot assign @djberg96

@kbrock
Copy link
Member

kbrock commented Jun 21, 2019

I had tried with #18876

But some caching got injected in there after a few runs.

It sounded like the consensus for the next version would:

  • remove check_concurrent_tasks from eligibility. since this changes over time
  • load the counts locally, and divvy out the tasks ignoring the counts in the database. Since the counts will only change by 1 and this method is the one that will change it

@djberg96 djberg96 removed their assignment Dec 18, 2019
@mfeifer mfeifer assigned ghost Jan 16, 2020
@miq-bot
Copy link
Member

miq-bot commented Jun 11, 2020

This issue has been automatically marked as stale because it has not been updated for at least 3 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the traige process documentation.

@miq-bot miq-bot added the stale label Jun 11, 2020
@Fryguy
Copy link
Member

Fryguy commented Jul 6, 2020

@fdupont-redhat Is this issue still relevant?

@ghost
Copy link
Author

ghost commented Jul 7, 2020

@Fryguy If we remove the V2V feature, it's not.

@gtanzillo gtanzillo added pinned and removed stale labels Jul 13, 2020
@gtanzillo gtanzillo removed the pinned label Sep 28, 2020
@Fryguy
Copy link
Member

Fryguy commented Dec 1, 2021

Removed in #21515

@Fryguy Fryguy closed this as completed Dec 1, 2021
@miq-bot
Copy link
Member

miq-bot commented Dec 1, 2021

@ghost 'djberg96' is an invalid assignee, ignoring...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants