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

Compare decimal columns correctly in batch saver #17020

Merged
merged 1 commit into from
Feb 26, 2018

Conversation

cben
Copy link
Contributor

@cben cben commented Feb 19, 2018

Extracted from #16722. Will allow container_quota_items.quota_* decimal columns to be used in manager_ref.

@Ladas Thanks for pointers last week! Please review.
cc @agrare

https://bugzilla.redhat.com/show_bug.cgi?id=1504560
@miq-bot add-label enhancement, gaprindashvili/yes

attribute_type = @model_class.type_for_attribute(key.to_s)
pg_type = @pg_types[key.to_sym]

if inventory_collection.use_ar_object?
# When using AR object, lets make sure we type.serialize(value) every value, so we have a slow but always
# working way driven by a configuration
obj[key.to_sym] = attribute_type
@serializable_keys[key.to_sym] = attribute_type
@deserializable_keys[key.to_sym] = attribute_type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I thought this unnecessary, but then I wrote the test and it failed with {:saver_strategy=>:batch, :use_ar_object=>true}
(duh, that's the one container refresh tests don't exercise 😉).
Turns out map_ids_to_inventory_objects ignores use_ar_object and takes values from DB directly anyway (unless it has to use fallback query mode, with unrelated conditions).
So I set deserializable_keys in use_ar_object mode too.
Should I rewrite map_ids_to_inventory_objects to check use_ar_object?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should I rewrite map_ids_to_inventory_objects to check use_ar_object?

Yeah, sound like we have to do it. And we should add 'use_ar_object' to containers specs.

Will allow container_quota_items.quota_* columns to be used in manager_ref.
https://bugzilla.redhat.com/show_bug.cgi?id=1504560
@miq-bot
Copy link
Member

miq-bot commented Feb 19, 2018

Checked commit cben@500fb84 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 8 offenses detected

spec/models/manager_refresh/save_inventory/saver_strategies_spec.rb

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.

looks good

@agrare the point is to do simple backportable change for now. For upstream we will refactor this to more generic solution. Problem is that the generic solution will be slower, so it would be nice if we could enable the parallel saving.

@Ladas
Copy link
Contributor

Ladas commented Feb 20, 2018

@miq-bot assign @agrare

@agrare agrare merged commit d3f7348 into ManageIQ:master Feb 26, 2018
@agrare agrare added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 26, 2018
simaishi pushed a commit that referenced this pull request Mar 22, 2018
Compare decimal columns correctly in batch saver
(cherry picked from commit d3f7348)

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

Gaprindashvili backport details:

$ git log -1
commit 6864c7f34a52560658448dd73bd4ee66e245a8f3
Author: Adam Grare <agrare@redhat.com>
Date:   Mon Feb 26 09:38:22 2018 -0500

    Merge pull request #17020 from cben/deserializable_keys
    
    Compare decimal columns correctly in batch saver
    (cherry picked from commit d3f73488d6fc9ad878ae9192c5437ba6916d588e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1559544

@agrare
Copy link
Member

agrare commented Mar 23, 2018

@cben this caused issues in the G-branch ref due to depending on some of the refactoring that @Ladas did in master that is not in gaprindashvili

@simaishi
Copy link
Contributor

After discussing with @agrare, reverted the backport from Gaprindashvili branch.

commit 9eab6ed7184247107950a79187c496e80f5e3026
Author: Satoe Imaishi <simaishi@redhat.com>
Date:   Fri Mar 23 14:24:35 2018 -0400

    Revert "Merge pull request #17020 from cben/deserializable_keys"
    
    This reverts commit 6864c7f34a52560658448dd73bd4ee66e245a8f3.
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1559544

@simaishi
Copy link
Contributor

Marking as conflict for now.

cben added a commit to cben/manageiq that referenced this pull request Mar 27, 2018
…zable_keys""

This reverts commit 9eab6ed,
again merging pull request ManageIQ#17020 from cben/deserializable_keys

Compare decimal columns correctly in batch saver
(cherry picked from commit d3f7348)

https://bugzilla.redhat.com/show_bug.cgi?id=1559544
cben added a commit to cben/manageiq that referenced this pull request Mar 28, 2018
…zable_keys""

This reverts commit 9eab6ed,
again merging pull request ManageIQ#17020 from cben/deserializable_keys

Compare decimal columns correctly in batch saver
(cherry picked from commit d3f7348)

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

Backported to Gaprindashvili via #17214

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