Do not keep all association records in the memory #14066
Conversation
|
This pull request is not mergeable. Please rebase and repush. |
Ladas
force-pushed
the
do_not_keep_all_association_records_in_the_memory
branch
2 times, most recently
from
858fcdf
to
61f4dd3
Compare
Ladas
force-pushed
the
do_not_keep_all_association_records_in_the_memory
branch
from
61f4dd3
to
dc824d2
Compare
| if inventory_object.nil? | ||
| next unless inventory_collection.delete_allowed? | ||
| deleted_counter += 1 | ||
| record.public_send(inventory_collection.delete_method) |
There was a problem hiding this comment.
does this read better for you?
delete_counter += 1 if inventory_collection.delete_record(record)
def delete_record(record)
return false unless inventory_collection.delete_allowed?
record.public_send(delete_method)
true
end| record = inventory_collection.model_class.create!(hash.except(:id)) | ||
| created_counter += 1 | ||
|
|
||
| inventory_object.id = record.try(:id) |
There was a problem hiding this comment.
think record will always have a value here (do not try, do)
There was a problem hiding this comment.
the .try was there because of the old refresh (took it from there). We don't do create! and update! there, so it can happen the record is not created.
| index = inventory_collection.object_index_with_keys(unique_index_keys, record) | ||
| if record_index[index] | ||
| # We have a duplicate in the DB, destroy it | ||
| record_for_destruction = nil |
There was a problem hiding this comment.
thinking out loud: (I might have swapped my logic)
record_for_destruction, record_index[index] = prune_duplicates(record, record_index[index])
if record_for_destruction
log
record_for_destruction.destroy
end
# typical:
def prune_duplicates(record, previous_record)
if previous_record
[previous_record, record]
else
return [record, record]
end
end
# the service case would be different.from here, may be able to consolidate the record.service code - not sure if you can move that into a inventory_collection or something. But at least it will be compact.
|
This pull request is not mergeable. Please rebase and repush. |
|
cool. dependency is merged. rebase away |
Do not keep a big amount of AR objects in memory, by turning around the alghoritm, we can load AR objects in batches and just keep our InventoryObject + hashes with data in memory.
Remove .try(:id) and just use .id, we create and update records with ! method name, so it should never be a nil.
Extract methods for create/delete/update
Ladas
force-pushed
the
do_not_keep_all_association_records_in_the_memory
branch
from
dc824d2
to
b649f9e
Compare
|
Do not change. just something I noticed:
unique_index_keys = inventory_collection.manager_ref_to_cols
# ...
index = inventory_collection.object_index_with_keys(unique_index_keys, record)may want to keep in the Also, regarding those |
| else | ||
| record_index[index] = record | ||
| end | ||
| attributes_index[index] = attributes |
There was a problem hiding this comment.
future: merge attributes = ... onto this line?
There was a problem hiding this comment.
hm I think I need to make the attributes!, the order here is important
|
|
||
| def update_record!(inventory_collection, record, hash, inventory_object) | ||
| record.assign_attributes(hash.except(:id, :type)) | ||
| if inventory_collection.check_changed? |
There was a problem hiding this comment.
not sure if this is easier to read:
(again for future PR if you like it)
record.save if !inventory_collection.check_changed? || record.changed?
| inventory_object = inventory_objects_index.delete(index) | ||
| hash = attributes_index.delete(index) | ||
|
|
||
| if inventory_object.nil? |
There was a problem hiding this comment.
this confuses me
I expect if inventory_object exists, then to possible delete.
could you put a comment above this check?
There was a problem hiding this comment.
right so, this is a case when it was found in MIQ DB but not in API
Add nice comments for refresh saving workflow
Doing the condition for check_changed? inline
|
Checked commits Ladas/manageiq@fb0d86d~...37c2b3f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
Depends on
This is optimization of second+ refresh, where we always load everything we have in our DB, in order to figure out what needs to be created/delete/updated by comparing to data we got from the API.
The biggest difference is visible on large table, e.g. a lot of templates, s3 objects, etc.
so this can save a lot of memory for large tables (in the real example, the memoory spikes to 1.1GB and does not get GCed, because of aws-sdk)