[wip] foreman refresh: use object refs instead of ids

[kbrock]

Save inventory converts a hash to a collection of active record objects.
Sometimes one of these collections links to another.

pseudo code for linking records:

  save_inventory_multi(code_lookup_hash, :find_id => [:ems_ref])
  store_ids_for_new_records(code_lookup_hash, :find_id => [:ems_ref])
  target_hash.each {|tgt| tgt[:code_id] = code_lookup_hash.detect {|c| c.ems_ref == tgt[:code_ref]}[:id]}
  save_inventory_multi(target_hash, :ignore => :code_ref)

If it is a self reference:

  save_inventory_multi(obj, :hosts, target_hash, :find_id => [:ems_ref], :ignore => :parent)
  store_ids_for_new_records(obj.hosts, target_hash, :find_id => [:ems_ref])
  obj.hosts.each do |o|
    o.update_attributes(:parent_id => target_hash[o.id][:parent][:id])
  end

New suggested code is to just store the active record reference in the hash instead of the id:

1. no need to saving the record first
2. no need to store_ids_for_new_records which loops through all active record objects for lookup.
3. use the active record association rather than using the id.
4. only 1 line change in core code.
5. No need to change any existing code, but it would make sense to leverage it.

Gave example how this affects foreman. We have another PR #2440 that leverages this

/cc @gmcculloug @brandondunne @Fryguy

[miq-bot]

Checked commit kbrock@2053118 with rubocop 0.27.1
3 files checked, 2 offenses detected

vmdb/app/models/ems_refresh/save_inventory_helper.rb

• 🔹 - Line 2, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for save_inventory_multi is too high. [19.82/15]
• 🔹 - Line 2, Col 27 - Metrics/ParameterLists - Avoid parameter lists longer than 5 parameters.

[Fryguy]

Can you show an example of the database queries (you can get them running through console). I think this is ok (after GregM explained to me), but I'm concerned about thrashing. Or perhaps, just run it twice in console and verify you don't have thrashing on records.

[kbrock]

@Fryguy good call gist

Yes, confirmed, no thrashing.

Not happy with all the duplicate lookups.
Really looks like we are calling association(true) too many times in our code. But that is the nature of the refresh.

[kbrock]

@blomquisg had concern with storing ar_object in the hash.
I think this is just a logical progression from storing :id in the hash.

In the future, it would be nice if save_inventory_multi returned something like [[hash, ar_object],[hash, ar_object]] - but that is probably for another day

[kbrock]

@blomquisg @Fryguy Does this meet your concerns? If not, just say so.

(this PR is blocking 2440 that is blocking 1 or 2 others) I can rewrite 2440 to not leverage this and move on.

[Fryguy]

Loading the locations and organizations over and over again is a form of thrashing. It should not be necessary if the save_inventory chain is done correctly. However those are not affected by the changes in this PR and so were there already.

What might be affected by this PR is that configuration_script loading over and over again...that should not be happening. Did that happen when you were use ids? Was there a specific reason you didn't use ids like we do elsewhere?

[kbrock]

Thrashing is not from this PR.

This thrashing is due to a record having a second habtm layer.
When we were loading these in a single manager, it was easy. no need to lookup all of these in the database, just once at the REST layer and sync each relationship to the database. (the id lookup happened automatically as part of the sync)

Since this is loading at a second level habtm, each of the children loads the relationships. I'll look into include and see if removing the relation(reload=true) helps this.

[Fryguy]

See how we already handle habtm and just do the same thing. Essentially
we save one thing first completely to the database, then when the second
thing comes along we just link back to the existing records. This there
is no need for double lookups or the N+1.

If you have N+1 at the rest layer that should not translate down to the
save inventory.
On Apr 1, 2015 12:02 PM, "Keenan Brock" notifications@github.com wrote:

Thrashing is not from this PR.

This thrashing is due to a record having a second habtm layer.
When we were loading these in a single manager, it was easy. no need to
lookup all of these in the database, just once at the REST layer and sync
each relationship to the database. (the id lookup happened automatically as
part of the sync)

Since this is loading at a second level habtm, each of the children loads
the relationships. I'll look into include and see if removing the
relation(reload=true) helps this.

—
Reply to this email directly or view it on GitHub
#2473 (comment).

[kbrock]

Is this problem related to this PR?

I am looking into it, but wondering if that should be blocking this PR

[kbrock]

Going to return objects from save_inventory
This will allow us to go many directions from there