Allow to run post processing job for ManagerRefresh (Graph Refresh) #15678
Conversation
Allow to run post processing job for ManagerRefresh (Graph Refresh). This post processing can leverage existing data in the InventoryCollection and especially each collection's created_records, updated_records and deleted_records, to queue up specific post processing jobs.
|
Checked commit Ladas@04f82ac with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/models/ems_refresh/refreshers/ems_refresher_mixin.rb
|
| end | ||
| end | ||
|
|
||
| def manager_refresh_post_processing(_ems, _target, _inventory_collections) |
There was a problem hiding this comment.
Can we re-use the existing post_refresh method? If you need the inv collections maybe we could move that into the refresh_targets_for_ems?
There was a problem hiding this comment.
From what I see we can't, the current post refresh is called to late, so at a point where all InventoryCollections are thrown away. And we cannot simply store them, since it goes over many targets, so there is possibility of having more Persiters there
There was a problem hiding this comment.
Yeah I was wondering if we could move the existing post_refresh here, but this is per target and that is global so not a great fit.
There was a problem hiding this comment.
yeah, the old refresh is kind of disconnected from the processing, so only thing we can do (and are doing) are timestamp based queries to figure out what have changed
| @@ -90,9 +91,20 @@ def refresh_targets_for_ems(ems, targets) | |||
|
|
|||
| Benchmark.realtime_block(:save_inventory) { save_inventory(ems, target, hashes) } | |||
| _log.info "#{log_header} Refreshing target #{target.class} [#{target.name}] id [#{target.id}]...Complete" | |||
|
|
|||
| if hashes.kind_of?(Array) | |||
There was a problem hiding this comment.
I really dislike checking if hashes is an array to imply manager_refresh, we already do that here save_ems_inventory and I don't want to add another.
There was a problem hiding this comment.
yeah, it's a funky condition, but we don't really have better. We could extract that condition somewhere though.
There was a problem hiding this comment.
Yeah I was wondering if we could move the existing post_refresh here, but this is per target and that is global so not a great fit.
Responded to the wrong comment
There was a problem hiding this comment.
I think we should at least be sending a ManagerRefresh::Inventory::Persister instead of an Array here so it is more obvious what case we are dealing with, but that can be a follow-up refactoring PR lets just not lose track of it
There was a problem hiding this comment.
right, but the problem is that we throw away the Persister too now. All we send is a list of ICs. Might be good to refactor it all to pass Persister though.
There was a problem hiding this comment.
Yeah I was thinking if instead of sending the inventory collections here we could just send the persister but I'll need to play around with it a bit
There was a problem hiding this comment.
yes, that would make sense
Allow to run post processing job for ManagerRefresh (Graph Refresh).
This post processing can leverage existing data in the
InventoryCollection and especially each collection's created_records,
updated_records and deleted_records, to queue up specific post
processing jobs.