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

Add save hooks on MiqReportResult to remove groupings #17598

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Jun 15, 2018

The report.extras[:groupings] on MiqReportResult were being serialized to the report column, and on certain chargeback reports, can get quite large. This data is not needed, so we remove it prior to saving.

The after_commit exists in case there is a use for the saved data after it has been saved and still used within the original instance of the MiqReportResult for building the result. Most likely this is overkill, but for now it is in place as a safety measure.

Links

@@ -35,6 +35,9 @@ class MiqReportResult < ApplicationRecord
end
end

before_save { @_extra_groupings = report.extras.delete(:grouping) }
after_commit { report.extras[:grouping] = @_extra_groupings }
Copy link
Member Author

Choose a reason for hiding this comment

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

Worth noting: This has to be an after_commit instead of a after_save, otherwise it puts the ActiveRecord object into a dirty state, and causes an infinite loop. If this gets changed to the latter, the test should catch it.

Copy link
Member

Choose a reason for hiding this comment

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

What is this witchcraft... So, are we removing it before we commit it to the DB but put it back into the in-memory object?

Copy link
Member

Choose a reason for hiding this comment

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

Should we clear @_extra_groupings after?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, are we removing it before we commit it to the DB but put it back into the in-memory object?

Yup, basically. Trying to play it safe just incase I missed something where this is used after it is saved.

Should we clear @_extra_groupings after?

I had that in there previously, but nothing is using that var, so I am tempted to just not bother. We aren't .duping it, so just passing a reference of it around. Up to you though if you feel strongly one way or the other.

@jrafanie
Copy link
Member

can get quiet large

quite

@NickLaMuro NickLaMuro force-pushed the prevent_groupings_from_being_saved_on_report_results branch from 9f21198 to 071fd83 Compare June 15, 2018 22:01
@NickLaMuro
Copy link
Member Author

can get quiet large

quite

@jrafanie quiet you...

@NickLaMuro
Copy link
Member Author

Test failures are from this PR. Getting them fixed.

@NickLaMuro NickLaMuro force-pushed the prevent_groupings_from_being_saved_on_report_results branch 2 times, most recently from 4cd1f28 to af169b6 Compare June 16, 2018 02:58
@@ -35,6 +35,9 @@ class MiqReportResult < ApplicationRecord
end
end

before_save { @_extra_groupings = report.extras.delete(:grouping) if report.kind_of?(MiqReport) }
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this needs a short comment about why we're doing this and a link to the BZ.

@@ -105,6 +105,23 @@
expect(report_result.report_results.table.data).not_to be_nil
end

it "should not include `extras[:grouing]` in the report column" do
Copy link
Member

Choose a reason for hiding this comment

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

typo: grouping

Copy link
Member Author

Choose a reason for hiding this comment

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

Butts... will fix

@jrafanie
Copy link
Member

LGTM with the minor nitpicks I suggested.

@NickLaMuro NickLaMuro force-pushed the prevent_groupings_from_being_saved_on_report_results branch from af169b6 to e534eb2 Compare June 18, 2018 16:09
@JPrause
Copy link
Member

JPrause commented Jun 18, 2018

@miq-bot add_label blocker

@JPrause
Copy link
Member

JPrause commented Jun 18, 2018

@NickLaMuro if this can be backported, please add the label gaprindashvili/yes

@@ -35,6 +35,17 @@ class MiqReportResult < ApplicationRecord
end
end

# These two hooks of a fix for the following BZ:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like this: These two hooks prevent temporary chargeback aggregation data to be retained in each miq_report_results row found in the following BZ:

#
# https://bugzilla.redhat.com/show_bug.cgi?id=1590908
#
# The remove extra data that is not necessary to be saved to the DB, but
Copy link
Member

@jrafanie jrafanie Jun 18, 2018

Choose a reason for hiding this comment

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

Is The remove a typo? Is it supposed to be These remove

# https://bugzilla.redhat.com/show_bug.cgi?id=1590908
#
# The remove extra data that is not necessary to be saved to the DB, but
# allow it to be retained for the remainder of the instanation incase the
Copy link
Member

Choose a reason for hiding this comment

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

instantiation
in case

# The remove extra data that is not necessary to be saved to the DB, but
# allow it to be retained for the remainder of the instanation incase the
# data is used to finish building the report results (the `after_commit` hook
# specifically, most likely this is overkill.
Copy link
Member

Choose a reason for hiding this comment

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

the after_commit hook specifically, is probably overkill but allows us to not break existing code that expects the temporary chargeback data in the instantiated object

^ Maybe?

@NickLaMuro NickLaMuro force-pushed the prevent_groupings_from_being_saved_on_report_results branch 3 times, most recently from 0bfcd41 to 0addd38 Compare June 18, 2018 18:48
https://bugzilla.redhat.com/show_bug.cgi?id=1590908

The `report.extras[:groupings]` on MiqReportResult were being
serialized to the `report` column, and on certain chargeback reports,
can get quite large.  This data is not needed, so we remove it prior to
saving.

The `after_commit` exists in case there is a use for the saved data after
it has been saved and still used within the original instance of the
MiqReportResult for building the result.  Most likely this is overkill,
but for now it is in place as a safety measure.

Also, `after_commit` is used because a `after_save` will cause an
"stack level too deep" error since editing the report value causes the
record to be put into a `dirty` state, and the ActiveRecord internals
don't like that.
@NickLaMuro NickLaMuro force-pushed the prevent_groupings_from_being_saved_on_report_results branch from 0addd38 to 253cf7a Compare June 18, 2018 18:57
@jrafanie
Copy link
Member

Line 50, Col 26 - Style/AsciiComments - Use only ascii symbols in comments.

Wow, bot is harsh

@miq-bot
Copy link
Member

miq-bot commented Jun 18, 2018

Checked commit NickLaMuro@253cf7a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@NickLaMuro
Copy link
Member Author

@miq-bot add_label reporting, enhancement, gaprindashvili/yes

@carbonin carbonin self-assigned this Jun 18, 2018
@carbonin carbonin merged commit 28dc337 into ManageIQ:master Jun 18, 2018
@carbonin carbonin added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 18, 2018
@jrafanie
Copy link
Member

@miq-bot add_label fine/yes

simaishi pushed a commit that referenced this pull request Jun 22, 2018
…g_saved_on_report_results

Add save hooks on MiqReportResult to remove groupings
(cherry picked from commit 28dc337)

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

Fine backport details:

$ git log -1
commit 49df80e32e4ebf2646b5cd759430e5db3df29d7e
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Mon Jun 18 18:03:39 2018 -0400

    Merge pull request #17598 from NickLaMuro/prevent_groupings_from_being_saved_on_report_results
    
    Add save hooks on MiqReportResult to remove groupings
    (cherry picked from commit 28dc33781dbe888dd8b0aaff4ada7af070891f7e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1594387

simaishi pushed a commit that referenced this pull request Jun 22, 2018
…g_saved_on_report_results

Add save hooks on MiqReportResult to remove groupings
(cherry picked from commit 28dc337)

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

Gaprindashvili backport details:

$ git log -1
commit d80c21a173aea3c0249dbea0ba0039428d44290d
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Mon Jun 18 18:03:39 2018 -0400

    Merge pull request #17598 from NickLaMuro/prevent_groupings_from_being_saved_on_report_results
    
    Add save hooks on MiqReportResult to remove groupings
    (cherry picked from commit 28dc33781dbe888dd8b0aaff4ada7af070891f7e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1594386

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.

6 participants