Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fixes #2498 FactNames are not precreated #615

Closed
wants to merge 1 commit into from
Closed

fixes #2498 FactNames are not precreated #615

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 6 additions & 10 deletions app/models/host/base.rb
View file
Open in desktop
Expand Up @@ -82,21 +82,17 @@ def merge_facts(facts)
FactValue.delete(deletions) unless deletions.empty?

# Get FactNames in one call
fact_names = FactName.where(:name => facts.keys)
fact_names = FactName.maximum(:id, :group => 'name')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my limited testing

fact_names = Hash[FactName.reorder('').group(:name).select('DISTINCT(name), id').map{|f| [f.name, f.id]}]

seems faster


# Create any needed new FactNames
facts.keys.dup.delete_if { |n| fact_names.map(&:name).include? n }.each do |needed|
fact_names << FactName.create(:name => needed)
end

# Lastly, add any new parameters.
fact_names.each do |fact_name|
next if db_facts.include?(fact_name.name)
value = facts[fact_name.name]
facts.each do |name, value|
next if db_facts.include?(name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of comparing each run, cant we also optimize it, e.g. something in the spirit of:

(facts.keys - db_facts).each do |name|
  value = facts[name]

values = value.is_a?(Array) ? value : [value]

values.each do |v|
next if v.nil?
fact_values.build(:value => v, :fact_name => fact_name)
fact_values.build(:value => v, :fact_name => fact_names.keys.include?(name) ?
fact_names[name] : FactName.create!(:name => name))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fact_name => fact_names[name] will always fail
as facts_names[name] is a hash, and you are expecting either a fact_name_id => facts[name] or a fact_name => FactName.find(facts[name]).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a very minimal fix for now (ignoring the db fact name dups)

diff --git a/app/models/host/base.rb b/app/models/host/base.rb
index ee068e7..75a44dc 100644
--- a/app/models/host/base.rb
+++ b/app/models/host/base.rb
     # Inspired from Puppet::Rails:Host
@@ -82,7 +83,7 @@ module Host
       FactValue.delete(deletions) unless deletions.empty?

       # Get FactNames in one call
-      fact_names = FactName.maximum(:id, :group => 'name')
+      fact_names = Hash[FactName.reorder('').group(:name).select('DISTINCT(name), id').map{|f| [f.name, f.id]}]

       # Create any needed new FactNames
       facts.each do |name, value|
@@ -91,8 +92,8 @@ module Host

         values.each do |v|
           next if v.nil?
-          fact_values.build(:value => v, :fact_name => fact_names.keys.include?(name) ?
-                                         fact_names[name] : FactName.create!(:name => name))
+          fact_values.build(:value => v, :fact_name_id => fact_names.keys.include?(name) ?
+                                         fact_names[name] : FactName.create!(:name => name).id)
         end
       end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why replacing :fact_name with :fact_name_id - those are equivalent?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use fact_name, then you expect a FactName instance.
if you pass a fact_name_id then you expect a numerical ID.

you can't mix between them (which is the current bug I was hitting).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I see now why.

end
end
end
Expand Down