-
Notifications
You must be signed in to change notification settings - Fork 897
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
Graph refresh skeletal precreate #16882
Changes from all commits
c0efa0c
7a339bb
5292e85
112380e
685f75a
75f13ad
9ef7619
3e8dc08
f588a29
bcb43fa
1513ad5
8cfba03
246ebb0
34b8d2b
beb322c
87cad0f
ee8be26
2395d5c
5efd919
eebc377
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
:association, :complete, :update_only, :transitive_dependency_attributes, :check_changed, :arel, | ||
:inventory_object_attributes, :name, :saver_strategy, :manager_uuids, :builder_params, | ||
:skeletal_manager_uuids, :targeted_arel, :targeted, :manager_ref_allowed_nil, :use_ar_object, | ||
:targeted_arel, :targeted, :manager_ref_allowed_nil, :use_ar_object, | ||
:created_records, :updated_records, :deleted_records, | ||
:custom_reconnect_block, :batch_extra_attributes, :references_storage | ||
|
||
|
@@ -107,8 +107,10 @@ class InventoryCollection | |
:find_by, | ||
:lazy_find, | ||
:lazy_find_by, | ||
:named_ref, | ||
:primary_index, | ||
:reindex_secondary_indexes!, | ||
:skeletal_primary_index, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's separate index now, since the processing is different. E.g. skeletal records are processed with ON CONFLICT DO NOTHING sql |
||
:to => :index_proxy | ||
|
||
delegate :table_name, | ||
|
@@ -367,7 +369,7 @@ class InventoryCollection | |
def initialize(model_class: nil, manager_ref: nil, association: nil, parent: nil, strategy: nil, | ||
custom_save_block: nil, delete_method: nil, dependency_attributes: nil, | ||
attributes_blacklist: nil, attributes_whitelist: nil, complete: nil, update_only: nil, | ||
check_changed: nil, arel: nil, builder_params: {}, | ||
check_changed: nil, arel: nil, builder_params: {}, create_only: nil, | ||
inventory_object_attributes: 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, | ||
|
@@ -387,6 +389,7 @@ def initialize(model_class: nil, manager_ref: nil, association: nil, parent: nil | |
@internal_attributes = %i(__feedback_edge_set_parent __parent_inventory_collections) | ||
@complete = complete.nil? ? true : complete | ||
@update_only = update_only.nil? ? false : update_only | ||
@create_only = create_only.nil? ? false : create_only | ||
@builder_params = builder_params | ||
@name = name || association || model_class.to_s.demodulize.tableize | ||
@saver_strategy = process_saver_strategy(saver_strategy) | ||
|
@@ -399,7 +402,6 @@ def initialize(model_class: nil, manager_ref: nil, association: nil, parent: nil | |
@manager_uuids = Set.new.merge(manager_uuids) | ||
@all_manager_uuids = all_manager_uuids | ||
@parent_inventory_collections = parent_inventory_collections | ||
@skeletal_manager_uuids = Set.new.merge(manager_uuids) | ||
@targeted_arel = targeted_arel | ||
@targeted = !!targeted | ||
|
||
|
@@ -489,6 +491,10 @@ def create_allowed? | |
!update_only? | ||
end | ||
|
||
def create_only? | ||
create_only | ||
end | ||
|
||
def saved? | ||
saved | ||
end | ||
|
@@ -497,6 +503,10 @@ def saveable? | |
dependencies.all?(&:saved?) | ||
end | ||
|
||
def parallel_safe? | ||
@parallel_safe_cache ||= %i(concurrent_safe concurrent_safe_batch).include?(saver_strategy) | ||
end | ||
|
||
def data_collection_finalized? | ||
data_collection_finalized | ||
end | ||
|
@@ -517,17 +527,19 @@ def object_index_with_keys(keys, record) | |
def noop? | ||
# If this InventoryCollection doesn't do anything. it can easily happen for targeted/batched strategies. | ||
if targeted? | ||
if parent_inventory_collections.nil? && manager_uuids.blank? && skeletal_manager_uuids.blank? && | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like it can use a helper method, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I have a noop? refactoring in followup PR |
||
# It's a noop Parent targeted InventoryCollection | ||
true | ||
elsif !parent_inventory_collections.nil? && parent_inventory_collections.all? { |x| x.manager_uuids.blank? } | ||
elsif !parent_inventory_collections.nil? && parent_inventory_collections.all? { |x| x.manager_uuids.blank? } && | ||
skeletal_primary_index.blank? | ||
# It's a noop Child targeted InventoryCollection | ||
true | ||
else | ||
false | ||
end | ||
elsif data.blank? && !delete_allowed? | ||
elsif data.blank? && !delete_allowed? && skeletal_primary_index.blank? | ||
# If we have no data to save and delete is not allowed, we can just skip | ||
true | ||
else | ||
|
@@ -738,12 +750,12 @@ def db_collection_for_comparison | |
targeted_arel.call(self) | ||
elsif manager_ref.count > 1 | ||
# TODO(lsmola) optimize with ApplicationRecordIterator | ||
hashes = extract_references(manager_uuids + skeletal_manager_uuids) | ||
hashes = extract_references(manager_uuids) | ||
full_collection_for_comparison.where(build_multi_selection_condition(hashes)) | ||
else | ||
ManagerRefresh::ApplicationRecordIterator.new( | ||
:inventory_collection => self, | ||
:manager_uuids_set => (manager_uuids + skeletal_manager_uuids).to_a.flatten.compact | ||
:manager_uuids_set => manager_uuids.to_a.flatten.compact | ||
) | ||
end | ||
else | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,10 +10,11 @@ class DataStorage | |
|
||
delegate :each, :size, :to => :data | ||
|
||
delegate :find, | ||
:primary_index, | ||
delegate :primary_index, | ||
:build_primary_index_for, | ||
:build_secondary_indexes_for, | ||
:named_ref, | ||
:skeletal_primary_index, | ||
:to => :index_proxy | ||
|
||
delegate :builder_params, | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would the manager_uuid not be present now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we e.g. build with :ems_ref => nil There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that something we want to allow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, this is compatible with current behavior, just moved. It will just result to nil |
||
data << inventory_object | ||
|
||
# TODO(lsmola) Maybe we do not need the secondary indexes here? | ||
|
@@ -53,27 +54,40 @@ def find_or_build(manager_uuid) | |
end | ||
|
||
def find_or_build_by(manager_uuid_hash) | ||
if !manager_uuid_hash.keys.all? { |x| manager_ref.include?(x) } || manager_uuid_hash.keys.size != manager_ref.size | ||
raise "Allowed find_or_build_by keys are #{manager_ref}" | ||
find_in_data(manager_uuid_hash) || build(manager_uuid_hash) | ||
end | ||
|
||
def find_in_data(hash) | ||
hash = enrich_data(hash) | ||
|
||
if manager_ref.any? { |x| !hash.key?(x) } | ||
raise "Needed find_or_build_by keys are: #{manager_ref}, data provided: #{hash}" | ||
end | ||
|
||
# Not using find by since if could take record from db, then any changes would be ignored, since such record will | ||
# not be stored to DB, maybe we should rethink this? | ||
primary_index.find(manager_uuid_hash) || build(manager_uuid_hash) | ||
uuid = ::ManagerRefresh::InventoryCollection::Reference.build_stringified_reference(hash, named_ref) | ||
primary_index.find(uuid) | ||
end | ||
|
||
def build(hash) | ||
hash = builder_params.merge(hash) | ||
inventory_object = new_inventory_object(hash) | ||
|
||
uuid = inventory_object.manager_uuid | ||
# Each InventoryObject must be able to build an UUID, return nil if it can't | ||
return nil if uuid.blank? | ||
# Return existing InventoryObject if we have it | ||
return primary_index.find(uuid) if primary_index.find(uuid) | ||
# We will take existing skeletal record, so we don't duplicate references for saving. We can have duplicated | ||
# reference from local_db index, (if we are using .find in parser, that causes N+1 db queries), but that is ok, | ||
# since that one is not being saved. | ||
uuid = ::ManagerRefresh::InventoryCollection::Reference.build_stringified_reference(hash, named_ref) | ||
inventory_object = skeletal_primary_index.delete(uuid) | ||
|
||
# We want to update the skeletal record with actual data | ||
inventory_object&.assign_attributes(hash) | ||
|
||
# Build the InventoryObject | ||
inventory_object ||= new_inventory_object(enrich_data(hash)) | ||
# Store new InventoryObject and return it | ||
push(inventory_object) | ||
inventory_object | ||
|
||
return inventory_object unless inventory_object.nil? | ||
|
||
# TODO(lsmola) prepare for changing behavior, build will return nil if it can't build or the record is already | ||
# there. Maybe we should even make build a private method. | ||
find_in_data(enrich_data(hash)) | ||
end | ||
|
||
def to_a | ||
|
@@ -123,6 +137,13 @@ def to_raw_data | |
end | ||
end | ||
end | ||
|
||
private | ||
|
||
def enrich_data(hash) | ||
# This is 25% faster than builder_params.merge(hash) | ||
{}.merge!(builder_params).merge!(hash) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
module ManagerRefresh | ||
class InventoryCollection | ||
module Index | ||
module Type | ||
class Skeletal < ManagerRefresh::InventoryCollection::Index::Type::Base | ||
def initialize(inventory_collection, index_name, attribute_names, primary_index) | ||
super | ||
|
||
@primary_index = primary_index | ||
end | ||
|
||
delegate :builder_params, | ||
:new_inventory_object, | ||
:named_ref, | ||
:to => :inventory_collection | ||
|
||
delegate :blank?, | ||
:each, | ||
:to => :index | ||
|
||
# Find value based on index_value | ||
# | ||
# @param index_value [String] a index_value of the InventoryObject we search for | ||
# @return [InventoryObject|nil] Returns found value or nil | ||
def find(index_value) | ||
index[index_value] | ||
end | ||
|
||
# Deletes and returns the value on the index_value | ||
# | ||
# @param index_value [String] a index_value of the InventoryObject we search for | ||
# @return [InventoryObject|nil] Returns found value or nil | ||
def delete(index_value) | ||
index.delete(index_value) | ||
end | ||
|
||
# Builds index record with skeletal InventoryObject and returns it, or returns nil if it's already present | ||
# in primary_index or skeletal_primary_index | ||
# @param attributes [Hash] Skeletal data of the index, must contain unique index keys and everything else | ||
# needed for creating the record in the Database | ||
# @return [InventoryObject|nil] Returns built value or nil | ||
def build(attributes) | ||
attributes = {}.merge!(builder_params).merge!(attributes) | ||
|
||
# If the primary index is already filled, we don't want populate skeletal index | ||
uuid = ::ManagerRefresh::InventoryCollection::Reference.build_stringified_reference(attributes, named_ref) | ||
return if primary_index.find(uuid) | ||
|
||
# Return if skeletal index already exists | ||
return if index[uuid] | ||
|
||
# We want to populate a new skeletal index | ||
inventory_object = new_inventory_object(attributes) | ||
index[inventory_object.manager_uuid] = inventory_object | ||
end | ||
|
||
private | ||
|
||
attr_reader :primary_index | ||
end | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would we want
:create_only
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can now enforce to do only insert or upsert by this (so we will avoid loading a lot of records)