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
Fixes #20167 - speed up errata api #6844
Conversation
Issues: #20167 |
I get different counts for the API call with before/after (new on bottom) 1883,1884c1499,1500
< "subtotal": 2496,
< "total": 2496
---
> "subtotal": 19093,
> "total": 19093 This is on a Katello 3.0 fwiw. What's also odd is if I grep for "errata_id" in the results of both, the sets of IDs are different. |
|
@beav updated! |
app/models/katello/erratum.rb
Outdated
joins("INNER JOIN #{Katello::ContentFacetRepository.table_name} on \ | ||
#{Katello::ContentFacetRepository.table_name}.content_facet_id = #{Katello::ContentFacetErratum.table_name}.content_facet_id"). | ||
joins("INNER JOIN #{Katello::RepositoryErratum.table_name} AS host_repo_errata ON \ | ||
host_repo_errata.erratum_id = #{Katello::Erratum.table_name}.id AND \ | ||
#{Katello::ContentFacetRepository.table_name}.repository_id = host_repo_errata.repository_id") | ||
query = query.joins(:content_facet_errata) unless hosts | ||
|
||
query = query.joins(:content_facets).where("#{Katello::Host::ContentFacet.table_name}.host_id" => hosts.map(&:id)) if hosts | ||
query.uniq | ||
query = query.where("#{Katello::ContentFacetRepository.table_name}.content_facet_id" => hosts.joins(:content_facet)) if hosts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can the if hosts
and unless hosts
be changed to an if/else? it would make it more obvious that it's one or the other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, if/else
or ternary operator or whatever, your pick ⛏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to if/else.
There is zero diff in the data returned between the old and new ways 💯 It looks like if you grab just one page of data with 20 records, the speedup is about 5x or 10x (20 sec to 2-3 sec). If you grab the entire errata list, the speedup is "only" 2x but is still good. I added a small comment, but ACK otherwise. I don't think additional tests are needed only because the patch purposefully does not change any behavior. |
[test] |
1 similar comment
[test] |
@beav mind a once glance over over fixing those last suggestions? |
query = query.where("#{Katello::ContentFacetRepository.table_name}.content_facet_id" => hosts.joins(:content_facet)) | ||
else | ||
query = query.joins(:content_facet_errata) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is more readable, thx! 👓
@jlsherrill mega-ACK, thx! |
No description provided.