Batch saving strategy that does not require unique indexes #15627

Ladas · 2017-07-21T14:05:40Z

Batch saving strategy that does not require unique indexes. Using batched SQL we are able to achieve huge saving time improvement.

Ladas · 2017-07-21T14:07:52Z

@miq-bot assign @agrare
@miq-bot add_label enhancement

Add default_batch strategy allowing to do batched SQL while not being concurrent safe.

Add batch_extra_attributes parameter

Rename default_batch to batch saver strategy

Extract unique_index_columns and on_conflict_update as these are only 2 parts that differ between concurrent safe batch strategy using unique indexes and basic batch strategy.

Add batch_extra_attributes to the list of all_attribute_keys, which will allow us to save attributes that were not explicitely defined, but were created by model side effect.

Add basic batch strategy, this strategy does not require unique indexes. But it is not concurrent safe.

Do not duplicate deleted records

Always call Returning so we are able to ouptu :created_records

chessbyte · 2017-07-24T15:40:56Z

app/models/manager_refresh/inventory_collection.rb

    def initialize(model_class: nil, manager_ref: nil, association: nil, parent: nil, strategy: nil, saved: nil,
                   custom_save_block: nil, delete_method: nil, data_index: nil, data: nil, dependency_attributes: nil,
                   attributes_blacklist: nil, attributes_whitelist: nil, complete: nil, update_only: nil,
                   check_changed: nil, custom_manager_uuid: nil, custom_db_finder: nil, arel: nil, builder_params: {},
                   inventory_object_attributes: nil, unique_index_columns: nil, name: nil, saver_strategy: nil,
                   parent_inventory_collections: nil, manager_uuids: [], all_manager_uuids: nil, targeted_arel: nil,
                   targeted: nil, manager_ref_allowed_nil: nil, secondary_refs: {}, use_ar_object: nil,
-                   custom_reconnect_block: nil)
+                   custom_reconnect_block: nil, batch_extra_attributes: [])


Seems like there are a ridiculous number of parameters to this initialize method. Sandy Metz recommends no more than 4. Feels like this is a bad pattern that can be refactored in future PRs. /cc @agrare

@chessbyte indeed, we are at the point where this should be a final list, that should cover all the crazy corner cases we have. :-)

So next will be to refactor these, part will got to the Settings, rest should be divided into more objects we will pass here(also consuming settings). There are already several areas we can group, e..g Saver, DatabaseLoader, AttributesBuilder, RecordsMatcher, etc.

cben · 2017-07-24T15:50:57Z

extenuating circumstances: - For many purposes, it's a struct (largely immutable, but not completely). There is a lot of added methods, but I [nearly?] all constructor args can be read back. - As a user of this class, I think of it as a declarative DSL. There are tons of options (quite well documented), but you don't need all at once, most of the time I set 4–6 of them. Do you have any concrete ideas how a better interface could look?

Use primary_key for batch update matching, because it's simpler and much faster for processing.

miq-bot · 2017-07-24T16:13:13Z

Checked commits Ladas/manageiq@72e9c38~...3127e3d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 2 offenses detected

app/models/manager_refresh/save_collection/saver/sql_helper.rb

❗ - Line 4, Col 7 - Rails/Delegate - Use delegate to define delegations.
❗ - Line 55, Col 25 - Style/PercentLiteralDelimiters - %-literals should be delimited by ( and ).

Ladas · 2017-07-24T16:33:30Z

@cben yes, #15627 (comment)

then the interface would look something like:

saver = ManagerRefresh::Saver.new(:strategy => batch, :check_changed => true, :delete_method => nil) 
InventoryCollection.new(:parent => ems, :association => vms, :manager_ref => [:ems_ref], :saver => saver)

# then extract another group ...

I've already did a quick walk-through with @agrare , it will get few more iterations to get the grouping and the naming right. I should have time now, since we are near feature complete solution. :-)

Also, I am still thinking about the right place to pass Settings, since that is tied to EMS, so maybe we will be passing ManagerRefresh::Settings.new(ems) and we will extract the right section there. Also I am thinking what everything should be in the settings and what belongs to Persistor. (so what needs to be dynamically changeable)

agrare · 2017-07-25T13:15:08Z

app/models/manager_refresh/inventory_collection.rb

+    # @param batch_extra_attributes [Array] Array of symbols marking which extra attributes we want to store into the
+    #        db. These extra attributes might be a product of :use_ar_object assignment and we need to specify them
+    #        manually, if we want to use a batch saving strategy and we have models that populate attributes as a side
+    #        effect.


@Ladas do you have an example of needing this currently?

for vm, when we change :raw_power_state, other attributes are set, so we need to have

:batch_extra_attributes => [:power_state, :state_changed_on, :previous_state],

hopefully this can be autodiscovered later, while passing the same specs

👍 thanks

agrare · 2017-07-25T13:32:12Z

app/models/manager_refresh/save_collection/saver/batch.rb

+      end
+
+      def on_conflict_update
+        false


~~Why can't we do on_conflict_update with this strategy?~~

Scratch that, needs unique indexes https://www.postgresql.org/docs/9.5/static/sql-insert.html#SQL-ON-CONFLICT

agrare

The rest looks good to me, I think we all know the parameter list is out of control and needs refactoring and we have plans for doing so.

miq-bot assigned agrare Jul 21, 2017

miq-bot added the enhancement label Jul 21, 2017

Ladas added 8 commits July 24, 2017 14:57

Add default_batch strategy allowing to do batched SQL

72e9c38

Add default_batch strategy allowing to do batched SQL while not being concurrent safe.

Add batch_extra_attributes parameter

cdb3058

Add batch_extra_attributes parameter

Rename default_batch to batch saver strategy

862820b

Rename default_batch to batch saver strategy

Extract unique_index_columns and on_conflict_update

a60bfa2

Extract unique_index_columns and on_conflict_update as these are only 2 parts that differ between concurrent safe batch strategy using unique indexes and basic batch strategy.

Add batch_extra_attributes to the list of all_attribute_keys

b753983

Add batch_extra_attributes to the list of all_attribute_keys, which will allow us to save attributes that were not explicitely defined, but were created by model side effect.

Add basic batch strategy

05cfb60

Add basic batch strategy, this strategy does not require unique indexes. But it is not concurrent safe.

Do not duplicate deleted records

25bf34f

Do not duplicate deleted records

Always call Returning so we are able to ouptu :created_records

f59cfcb

Always call Returning so we are able to ouptu :created_records

Ladas force-pushed the batch_saving_strategy_that_does_not_require_unique_indexes branch from 94c93dd to f59cfcb Compare July 24, 2017 12:57

chessbyte reviewed Jul 24, 2017

View reviewed changes

Use primary_key for batch update matching

3127e3d

Use primary_key for batch update matching, because it's simpler and much faster for processing.

agrare reviewed Jul 25, 2017

View reviewed changes

agrare approved these changes Jul 25, 2017

View reviewed changes

agrare merged commit 182b16b into ManageIQ:master Jul 25, 2017

agrare added the providers/inventory label Jul 25, 2017

agrare added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch saving strategy that does not require unique indexes #15627

Batch saving strategy that does not require unique indexes #15627

Ladas commented Jul 21, 2017

Ladas commented Jul 21, 2017

chessbyte Jul 24, 2017

Ladas Jul 24, 2017

cben commented Jul 24, 2017 via email

miq-bot commented Jul 24, 2017

Ladas commented Jul 24, 2017

agrare Jul 25, 2017

Ladas Jul 25, 2017

agrare Jul 25, 2017

agrare Jul 25, 2017 •

edited

agrare left a comment

Batch saving strategy that does not require unique indexes #15627

Batch saving strategy that does not require unique indexes #15627

Conversation

Ladas commented Jul 21, 2017

Ladas commented Jul 21, 2017

chessbyte Jul 24, 2017

Choose a reason for hiding this comment

Ladas Jul 24, 2017

Choose a reason for hiding this comment

cben commented Jul 24, 2017 via email

miq-bot commented Jul 24, 2017

Ladas commented Jul 24, 2017

agrare Jul 25, 2017

Choose a reason for hiding this comment

Ladas Jul 25, 2017

Choose a reason for hiding this comment

agrare Jul 25, 2017

Choose a reason for hiding this comment

agrare Jul 25, 2017 • edited

Choose a reason for hiding this comment

agrare left a comment

Choose a reason for hiding this comment

agrare Jul 25, 2017 •

edited