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

Load Lans efficiently in ProvisionWorkflow #249

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented May 3, 2018

Instead of preloading Lan records, splitting them up based on if they are shareable and instantiating entire ActiveRecord instances to only use the name column in a hash, use a specific query to gather and filter those values and generate a hash from that.

IMPORTANT NOTE: There will be a PR on ManageIQ/manageiq that will remove the preloading in app/models/miq_provision_virt_workflow.rb that this relied on to avoid an N+1. The idea is that we would merge any provider PRs first, and then the fix in the main repo so the performance impact during this is minimal.

Benchmarks

Since this is part of a multi-PR fix, benchmarks will be displayed in the main PR in the ManageIQ/manageiq repo. Links to the related PRs will be posted down below when available.

Links

@NickLaMuro NickLaMuro force-pushed the faster_lan_lookup_in_provision_virt_workflow branch from 36bafd8 to 0164087 Compare May 3, 2018 16:10
@NickLaMuro NickLaMuro force-pushed the faster_lan_lookup_in_provision_virt_workflow branch from 0164087 to 69673af Compare May 3, 2018 17:31
Instead of preloading Lan records, splitting them up based on if they
are shareable and instantiating entire ActiveRecord instances to only
use the name column in a hash, use a specific query to gather and filter
those values and generate a hash from that.
@miq-bot
Copy link
Member

miq-bot commented May 3, 2018

Checked commit NickLaMuro@69673af with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@agrare agrare self-assigned this May 3, 2018
Copy link
Contributor

@syncrou syncrou left a comment

Choose a reason for hiding this comment

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

@NickLaMuro - LGTM

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Yay. removed a to_miq_q AND the sharable to the query.

@agrare agrare merged commit 69673af into ManageIQ:master May 18, 2018
agrare added a commit that referenced this pull request May 18, 2018
…n_virt_workflow

Load Lans efficiently in ProvisionWorkflow
@agrare agrare added this to the Sprint 86 Ending May 21, 2018 milestone May 18, 2018
@NickLaMuro
Copy link
Member Author

NickLaMuro commented May 31, 2018

@miq-bot add_label performance, fine/yes, gaprindashvili/yes

Please backport this first, then #269 .

agrare pushed a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 15, 2019
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