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

[GAPRINDASHVILI] Use AREL for custom_attributes virtual_attributes in MiqReport ONLY #17629

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Jun 22, 2018

Backport rework of #17615 for gapindashvili

https://bugzilla.redhat.com/show_bug.cgi?id=1594027
https://bugzilla.redhat.com/show_bug.cgi?id=1594027

This is a reworking of all of the commits in:

https://github.com/ManageIQ/manageiq/pull/17615

Previous commits include:

0ad75c2
e7fd484
065405d

Allows MiqReport generation to use Arel columns for the custom_attribute cols, but not use virtual_attributes as the mechanism for it, since they are not supported in other places (like MiqExpression) in this version of the application.

This is a single commit for the changes, since they are not really the mechanism we want going forward, and allow for an easier backport for fine.

Original commit message below


Converts the virtual_column definitions in CustomAttributeMixin to support the arel attribute, and allow MiqReport#generate to take advantage of this, and avoid needing an extra include.

Other notable changes:

  • If all custom_attributes have arel backed virtual_attribute (aka virtual_column) methods, then the includes of :custom_attributes => {} will be removed from the Rbac query. A helper method was added to assist with this
  • The method defined for accessing the virtual_attribute from the instance will now check the attributes hash prior to executing the rest of the method, and return something if it has a key
  • Some shared values calculated in both the virtual_attribute method definition and would be in the custom_attribute_arel method have been extracted out so it is only calculated once (minor performance benefit).

Benchmarks

These benchmarks are currently a work in progress, and contain some incomplete data. Most of those inconsistencies are noted in below.

Tested on a local machine with the DB running on the same machine, so next to no network based latency for queries is observed.

Before

50 uniq query types executed. 212961 are just the N+1 fetching custom_attributes

ms queries query (ms) rows
472820 223205 80092.4 419419

Raw Benchmark.measure numbers:

281.100000  13.840000 294.940000 (322.186379)
280.330000  12.210000 292.540000 (318.992034)
278.170000  11.730000 289.900000 (316.135889)

After

While that patch was present during the benchmarks, most of what is done in this patch negates it's usefulness, so almost everything seeing here is from the changes in this pull request.

NOTE: 10141 of the total queries and ~3580ms of the query (ms) here are INSERT statements into miq_report_result_details.

ms queries query (ms) rows
41189 10220 5017.1 38435

Raw Benchmark.measure numbers:

 38.170000   1.270000  39.440000 ( 43.803676)
 34.660000   0.700000  35.360000 ( 38.079291)
 35.000000   0.730000  35.730000 ( 38.463884)

Links

@NickLaMuro NickLaMuro changed the title Backport rework of #17626 for gapindashvili [GAPRINDASHVILI] Use AREL for custom_attributes virtual_attributes in MiqReport ONLY Jun 22, 2018
@NickLaMuro
Copy link
Member Author

@miq-bot add_label blocker, core, enhancement, fine/yes

@NickLaMuro NickLaMuro force-pushed the custom_attributes_into_extra_cols_for_miq_reports_gapindashvili branch from a56d063 to 8df68fd Compare June 25, 2018 14:57
@JPrause
Copy link
Member

JPrause commented Jun 25, 2018

@NickLaMuro can you ping someone to review.

@NickLaMuro
Copy link
Member Author

@JPrause Tests are failing, so not worth their time at the moment. I am working on them as we speak.

@NickLaMuro NickLaMuro force-pushed the custom_attributes_into_extra_cols_for_miq_reports_gapindashvili branch from 8df68fd to f466d4b Compare June 25, 2018 21:00
@@ -284,7 +295,11 @@ def _generate_table(options = {})
:skip_counts => true,
)

rbac_opts[:extra_cols] = va_sql_cols unless va_sql_cols.nil? || va_sql_cols.empty?
if va_sql_cols.present? || custom_attributes_sql_cols.present?
Copy link
Member Author

Choose a reason for hiding this comment

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

At a glance, this probably looks like redundant if checks, but basically we want to have rbac_opts[:extra_cols to be either not set, or a populated array. Otherwise this fails in ar_virtual.rb with the over written .select with have code around.

@NickLaMuro NickLaMuro force-pushed the custom_attributes_into_extra_cols_for_miq_reports_gapindashvili branch from f466d4b to 138c3ce Compare June 25, 2018 21:12
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.

nice job Nick

@NickLaMuro
Copy link
Member Author

NickLaMuro commented Jun 25, 2018

Don't Merge!

Looks like I have one thing to fix causing the specs to fail. Will get to it shortly.

Update: Should be fixed now.

https://bugzilla.redhat.com/show_bug.cgi?id=1594027
https://bugzilla.redhat.com/show_bug.cgi?id=1594027

This is a reworking of all of the commits in:

    ManageIQ#17626

Previous commits include:

  0ad75c2
  e7fd484
  065405d

* * *

Allows MiqReport generation to a form of SQL for the custom_attribute
columns, but not use virtual_attributes as the mechanism for it since
they are not supported in other places (like `MiqExpression`) in this
version of the application.

See ManageIQ#17626 for more details.
@NickLaMuro NickLaMuro force-pushed the custom_attributes_into_extra_cols_for_miq_reports_gapindashvili branch from 138c3ce to ae523a7 Compare June 25, 2018 22:37
@miq-bot
Copy link
Member

miq-bot commented Jun 25, 2018

Checked commit NickLaMuro@ae523a7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

@simaishi simaishi merged commit d7d0f96 into ManageIQ:gaprindashvili Jun 26, 2018
@simaishi simaishi added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 26, 2018
simaishi added a commit that referenced this pull request Jun 26, 2018
…a_cols_for_miq_reports_gapindashvili

[GAPRINDASHVILI] Use AREL for custom_attributes virtual_attributes in MiqReport ONLY
(cherry picked from commit d7d0f96)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1594028
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit cb5a8cb8cf3a9365c4b2098192a3e86a0f2d2a72
Author: Satoe Imaishi <[email protected]>
Date:   Tue Jun 26 08:43:07 2018 -0400

    Merge pull request #17629 from NickLaMuro/custom_attributes_into_extra_cols_for_miq_reports_gapindashvili
    
    [GAPRINDASHVILI] Use AREL for custom_attributes virtual_attributes in MiqReport ONLY
    (cherry picked from commit d7d0f96a5e4cf27f093116e8f6982de220691483)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1594028

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