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

Migrate EmsRefresh.refresh queue args to data #107

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented Oct 25, 2017

Move existing EmsRefresh.refresh queue args to data for performance.

Related to: ManageIQ/manageiq#16271

@miq-bot miq-bot added the wip label Oct 25, 2017
@agrare agrare force-pushed the migrate_ems_refresh_refresh_queue_args_to_data branch 5 times, most recently from ad9ca52 to 5255386 Compare October 25, 2017 14:50
@agrare agrare changed the title [WIP] Migrate EmsRefresh.refresh queue args to data Migrate EmsRefresh.refresh queue args to data Oct 25, 2017
@agrare
Copy link
Member Author

agrare commented Oct 25, 2017

@miq-bot assign @Fryguy

/cc @Fryguy @Ladas please review

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

Awesome! 👍

@Fryguy
Copy link
Member

Fryguy commented Oct 26, 2017

As discussed, since Marshal.dump/load is not guaranteed to work across Ruby versions, we should bracket this around upgrades and just delete rows that can't be Marshaled (which if one fails they all will likely fail).

We may want to be resilient on the MiqQueue side as well...not sure we ever caught that error there.

@agrare agrare force-pushed the migrate_ems_refresh_refresh_queue_args_to_data branch 2 times, most recently from a20e837 to cdede24 Compare October 26, 2017 15:47
@agrare
Copy link
Member Author

agrare commented Oct 26, 2017

@Fryguy added tests with msg_data containing an empty array with marshal format version 3.0

@agrare agrare force-pushed the migrate_ems_refresh_refresh_queue_args_to_data branch from cdede24 to 2dfc78f Compare October 26, 2017 15:50
end

it "Handle invalid marshal format errors" do
# "\x03\x00\x00" is an empty array in Marshal version 3.0
Copy link
Member

Choose a reason for hiding this comment

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

Is there a version of this for the up migration?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure how to test Marshal.dump failing due to a version issue, ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me try putting an anonymous class in the args, that should do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Scratch that, that won't even serializing the value to args

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see...it's only the load part that fails. 👍

@miq-bot
Copy link
Member

miq-bot commented Oct 26, 2017

Checked commit agrare@2dfc78f with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 2 offenses detected

db/migrate/20171025122732_move_ems_refresh_args_to_data.rb

spec/migrations/20171025122732_move_ems_refresh_args_to_data_spec.rb

@Fryguy Fryguy merged commit 0261203 into ManageIQ:master Oct 26, 2017
@Fryguy Fryguy added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 26, 2017
@agrare agrare deleted the migrate_ems_refresh_refresh_queue_args_to_data branch October 26, 2017 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants