Graph refresh skeletal precreate #16882
Conversation
Ladas
force-pushed
the
graph_refresh_skeletal_precreate
branch
2 times, most recently
from
6eb7768
to
2856a9d
Compare
|
This pull request is not mergeable. Please rebase and repush. |
Ladas
force-pushed
the
graph_refresh_skeletal_precreate
branch
from
2856a9d
to
5e057e5
Compare
|
This pull request is not mergeable. Please rebase and repush. |
Optimize find_or_build abstracting everything into build method and doing faster UUID resolving. We need faster uuid resolving since the parallel saving methods will be calling this for every edge.
Abstract skeletal_precreate
Expose if reference is primary, pointing to primary index
Fix concurrent_safe strategy, TODO added if it makes sense to keep this strategy. Similar effect would be done having batch size configurable in the batch strategy.
Store index keys of the reference
Remove skeletal precreate from scanner, finding a better place for it.
Add skeletal precreate to lazy_find, doing this for each lazy_find removes the need for recursive scan, that would be needed in scanner. Also enhancing condition where skeletal-precreate can apply. Limiting it only to targeted parallel safe refresh. Only for collections that haven't been saved. And only for lazy_find pointing to primary key. This can be applied for full refresh, but the skeletal refresh can break occurences where we use only build. Since calling build second, will not fill missing attributes.
Allow skeletal pre-create for any parallel safe strategy, constraint for targeted is too limited.
Enhance build method to do index test and clean up behavior. 1. We check all keys needed for index are present, dropping need for sending only the indexes 2. Extracting find_in_data method and exposing it 3. TODO that build should not return found object
Add create only option
Introduce skeletal index, it allows us to keep skeletal records while being able to turn them to full records.
Use skeletal index for skeletal precreate
Add code for creating skeletal records from skeletal index
Allow on conflict do nothing for skeletal records
Delegate blank? to index data so we can test if it's empty
Allow index proxy to look into skeletal indexes, if the strategy is parallel safe and we search in primary index.
…he DB For skeletal records, we need to always fetch the primary keys from the DB, since they do ON CONFLICT DO NOTHING.
Allow skeletal records to fetch also attribute_references, so we can get also attributes referenced by :key
Remove skeletal_manager_uuids as we do not need it
Ladas
force-pushed
the
graph_refresh_skeletal_precreate
branch
from
5e057e5
to
5efd919
Compare
Ladas
changed the title
Fix Rubocop issues
|
Checked commits Ladas/manageiq@c0efa0c~...eebc377 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 app/models/manager_refresh/inventory_collection/index/type/skeletal.rb
app/models/manager_refresh/save_collection/saver/sql_helper.rb
|
| @@ -73,10 +73,10 @@ class InventoryCollection | |||
| attr_accessor :parent_inventory_collections | |||
|
|
|||
| attr_reader :model_class, :strategy, :attributes_blacklist, :attributes_whitelist, :custom_save_block, :parent, | |||
| :internal_attributes, :delete_method, :dependency_attributes, :manager_ref, | |||
| :internal_attributes, :delete_method, :dependency_attributes, :manager_ref, :create_only, | |||
There was a problem hiding this comment.
When would we want :create_only?
There was a problem hiding this comment.
we can now enforce to do only insert or upsert by this (so we will avoid loading a lot of records)
| all_manager_uuids.nil? && parent_inventory_collections.blank? && custom_save_block.nil? | ||
| if parent_inventory_collections.nil? && manager_uuids.blank? && | ||
| all_manager_uuids.nil? && parent_inventory_collections.blank? && custom_save_block.nil? && | ||
| skeletal_primary_index.blank? |
There was a problem hiding this comment.
This looks like it can use a helper method, inventory_blank? or something
There was a problem hiding this comment.
Right, I have a noop? refactoring in followup PR
| :primary_index, | ||
| :reindex_secondary_indexes!, | ||
| :skeletal_primary_index, |
There was a problem hiding this comment.
What's the difference between the skeletal_primary_index and the primary_index? If we're using skeletal precreate can we just use the same index?
There was a problem hiding this comment.
it's separate index now, since the processing is different. E.g. skeletal records are processed with ON CONFLICT DO NOTHING sql
| @@ -31,7 +32,7 @@ def initialize(inventory_collection, secondary_refs) | |||
| end | |||
|
|
|||
| def <<(inventory_object) | |||
| unless primary_index.find(inventory_object.manager_uuid) | |||
| if inventory_object.manager_uuid.present? && !primary_index.find(inventory_object.manager_uuid) | |||
There was a problem hiding this comment.
Why would the manager_uuid not be present now?
There was a problem hiding this comment.
if we e.g. build with :ems_ref => nil
There was a problem hiding this comment.
Is that something we want to allow?
There was a problem hiding this comment.
right, this is compatible with current behavior, just moved. It will just result to nil
| SET #{all_attribute_keys_array.map { |key| build_insert_set_cols(key) }.join(", ")} | ||
| } | ||
| if inventory_collection.parallel_safe? | ||
| if on_conflict == :do_nothing |
There was a problem hiding this comment.
This looks like what the on_conflict_update method should be doing...can't we change that to
def on_conflict_action
:update
end
and override it instead of passing in :do_nothing as an arg? Or get rid of the on_conflict_update method and just use the arg. It is weird to mix both of those.
|
Couple of things that can be addressed in a followup PR |
Skeletal precreate is when we use graph edges to build skeletal nodes. Combining with unique indexes, we can use this to easily build the graph in parallel. Or build the graph out of order e.g. with targeted refresh, that would remove the need to scanning e.g. all Vms relations, to make sure it's being saved in the right order.