[AuthenticationMixin] authentication_status to virtual_delegate #18196
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kbrock
approved these changes
Nov 14, 2018
| @@ -4,7 +4,16 @@ module AuthenticationMixin | |||
| included do | |||
| has_many :authentications, :as => :resource, :dependent => :destroy, :autosave => true | |||
|
|
|||
| virtual_column :authentication_status, :type => :string | |||
| has_one :authentication_status_severity_level, | |||
NickLaMuro
added a commit
to NickLaMuro/manageiq-ui-classic
that referenced
this pull request
Nov 14, 2018
This is related to changes in: ManageIQ/manageiq#18196 When `authentication_status` is changed to a `virtual_delegate`, we are able to pre-fetch the values as part of the original query, and avoid an N+1.
NickLaMuro
changed the title
Fryguy
reviewed
Nov 15, 2018
Creates a virtual_delegate for AuthenticationMixin#authentication_status which allows it to be included in reports and put in as part of the main query. The biggest thing of note in this change is how the status_severity_arel is being used to handle the sorting properly in the SQL query. It makes use of a SQL's case statement to handle what would normally be a `sort_by` on the hash lookup of Authentication::STATUS_SEVERITY. Instead, we match each key to the status column and use that as the value for the `ORDER BY exp DESC` in the `has_one`, which allows for the same affect. This also means that it can inject itself in the `SELECT` of a `MiqReport`, and avoid N+1's on things like quadicons and such for pages that use `MiqReport#paged_view_search`.
NickLaMuro
force-pushed
the
authentication_mixin_authentication_status_to_virtual_delegate
branch
from
e02cf66 to
49d854d
Compare
|
Checked commit NickLaMuro@49d854d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
NickLaMuro
commented
Nov 15, 2018
| expect(host.authentication_status).to eq("None") | ||
| end | ||
|
|
||
| it "is 'None' when accessed via ruby, but fetched via sql" do |
There was a problem hiding this comment.
Worth noting: This spec and the one above have a important distinction to make here since it is slightly confusing when looking at the result that is provided by virtual_column_sql_value. The default value of "None" is desired here as we want to maintain the previous functionality when calling authentication_status.
|
sporadic failure strikes again - just kicked one travis job |
kbrock
reviewed
Nov 16, 2018
| # | ||
| STATUS_SEVERITY_AREL = Arel::Nodes::Case.new.tap do |arel_case| | ||
| STATUS_SEVERITY.each do |value, sort_weight| | ||
| arel_case.when(arel_table[:status].lower.eq(value)).then(sort_weight) |
There was a problem hiding this comment.
wow - turns out arel_table[:status] does not access the database.
who knew?
NickLaMuro
added a commit
to NickLaMuro/manageiq-ui-classic
that referenced
this pull request
Nov 20, 2018
This is related to changes in: ManageIQ/manageiq#18196 When `authentication_status` is changed to a `virtual_delegate`, we are able to pre-fetch the values as part of the original query, and avoid an N+1.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Creates a
virtual_delegateforAuthenticationMixin#authentication_statuswhich allows it to be included in reports and put in as part of the main query.The biggest thing of note in this change is how the
status_severity_arelis being used to handle the sorting properly in the SQL query. It makes use of a SQL's case statement to handle what would normally be asort_byon the hash lookup ofAuthentication::STATUS_SEVERITY. Instead, we match each key to the status column and use that as the value for theORDER BY exp DESCin thehas_one, which allows for the same affect.This also means that it can inject itself in the
SELECTof aMiqReport, and avoid N+1's on things like quadicons and such for pages that useMiqReport#paged_view_search.Metrics
An example set of runs I had with a 47
Hostprovider:Before
/ems_infra/report_dataAfter
/ems_infra/report_dataLinks
ComplianceMixin#last_compliance_statusauthentication_statusto theMiqReportin quertion (product/views/Host.yaml)Steps for Testing/QA
Here is a script to generate test data that I was using:
https://gist.github.com/NickLaMuro/225833358423723ed17ff294415fa6b4
For testing, I did the following:
manageiq:bin/rails r bz_1580569_db_replication_script.rbbin/rails sCompute -> Infrastructure -> Providersin the side menuper_pageto 1k (just to load all of the records in a single pageHostrecords, ideally)HostsbuttonBetween
master, and this change, you should see the number of queries per request for the last portion in the above drop by the number ofHostrecords for that provider. TheMetricssection above used this process.