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

Use database query to filter role memberships by type #1606

Merged
merged 1 commit into from
Jun 10, 2020

Conversation

micahlee
Copy link
Contributor

@micahlee micahlee commented Jun 9, 2020

What does this PR do?

  • What's changed? Why were these changes made?

Previously each membership role was queried individually from the database to
check its "kind" and find the layers. This led to massive performance degredation
with a large number of memberships, as is the case with a host factory creating
many (100s) of hosts.

This PR implements that filter entirely in SQL to avoid the round trips and
improve performance at scale.

What ticket does this PR close?

Connected to #1605

Checklists

Change log

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code changes, or
  • The changes in this PR do not require tests

Documentation

  • This PR does not require updating any documentation, or
  • Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs

Previously each membership role was queried individually from the database to
check its "kind" and find the layers. This led to massive performance degredation
with a large number of memberships, as is the case with a host factory creating
many (100s) of hosts.

This commit implements that filter entirely in SQL to avoid the round trips and
improve performance at scale.
@codeclimate
Copy link

codeclimate bot commented Jun 9, 2020

Code Climate has analyzed commit f953498 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 85.6% (0.0% change).

View more on Code Climate.

@nessiLahav nessiLahav self-requested a review June 10, 2020 08:27
Copy link
Contributor

@nessiLahav nessiLahav left a comment

Choose a reason for hiding this comment

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

Great work :) i validated your fix in my benchmark env, and WOWWW!!!

@micahlee
Copy link
Contributor Author

Thanks @nessiLahav, I'd love to actually see your before and after numbers.

I was testing this with a cucumber scenario that loaded 4000 hosts. After this, it completes in 5 minutes (with all of the setup and debug logging), but I couldn't get a before number because it would crash before finishing.

@micahlee micahlee merged commit fd273f8 into master Jun 10, 2020
@micahlee micahlee deleted the 1605-host-factory-performance branch June 10, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants