If you didn't have any delayed job workers and submitted multiple
reports for the same node, when you started multiple workers they would
each try to create the same node. There was no database constraint to
prevent this, and Rails validation was worthless since the workers were
running their own transactions.
By adding the database constraint the insert will fail for workers who
don't create the node first. Rather than discard the reports and create
an error for those workers, we're retrying the report submission 3
times before putting in a DelayedJobFailure entry.
Reviewed-by: Jacob Helwig <ja...@puppetlabs.com>
Signed-off-by: Matt Robinson <m...@puppetlabs.com>
---
Local-branch: ticket/1.2rc/8686
app/models/report.rb | 8 ++-
...28195829_add_node_host_uniqueness_constraint.rb | 15 ++++++
spec/models/report_spec.rb | 54 ++++++++++++++++++++
3 files changed, 75 insertions(+), 2 deletions(-)
create mode 100644
db/migrate/20110728195829_add_node_host_uniqueness_constraint.rb
diff --git a/app/models/report.rb b/app/models/report.rb
index 09c2b9e..95a5809 100644
--- a/app/models/report.rb
+++ b/app/models/report.rb
@@ -84,8 +84,12 @@ class Report < ActiveRecord::Base
File.unlink(report_file) if options[:delete]
return report
rescue Exception => e
- DelayedJobFailure.create!(:summary => "Importing report
#{File.basename(report_file)}",
- :details => e.to_s)
+ retries ||= 3
+ retry if (retries -= 1) > 0
+ DelayedJobFailure.create!(
+ :summary => "Importing report #{File.basename(report_file)}",
+ :details => e.to_s
+ )
return nil
end
diff --git a/db/migrate/20110728195829_add_node_host_uniqueness_constraint.rb
b/db/migrate/20110728195829_add_node_host_uniqueness_constraint.rb
new file mode 100644
index 0000000..dead775
--- /dev/null
+++ b/db/migrate/20110728195829_add_node_host_uniqueness_constraint.rb
@@ -0,0 +1,15 @@
+class AddNodeHostUniquenessConstraint < ActiveRecord::Migration
+ def self.up
+ execute <<-SQL
+ ALTER TABLE nodes
+ ADD CONSTRAINT uc_node_name UNIQUE (name)
+ SQL
+ end
+
+ def self.down
+ execute <<-SQL
+ ALTER TABLE nodes
+ DROP INDEX uc_node_name
+ SQL
+ end
+end
diff --git a/spec/models/report_spec.rb b/spec/models/report_spec.rb
index cf08f11..06dda9c 100644
--- a/spec/models/report_spec.rb
+++ b/spec/models/report_spec.rb
@@ -397,6 +397,60 @@ describe Report do
end
end
+ describe "#create_from_yaml_file" do
+ describe "when create_from_yaml is successful" do
+ before do
+ File.expects(:read).with('/tmp/foo').returns(@report_yaml)
+ Report.expects(:create_from_yaml).returns('i can haz report')
+ end
+
+ it "should call create_from_yaml on the file passed in and return the
results" do
+ Report.create_from_yaml_file('/tmp/foo').should == "i can haz report"
+ end
+
+ it "should delete the file if delete option is specified" do
+ File.expects(:unlink).with('/tmp/foo')
+ Report.create_from_yaml_file('/tmp/foo', :delete => true)
+ end
+ end
+
+
+ describe "when create_from_yaml fails" do
+ before do
+
File.expects(:read).at_least_once.with('/tmp/foo').returns(@report_yaml)
+ end
+
+ it "not unlink the file if create_from_yaml fails" do
+ File.expects(:unlink).with('/tmp/foo').never
+
Report.expects(:create_from_yaml).raises(ActiveRecord::StatementInvalid)
+ Report.create_from_yaml_file('/tmp/foo', :delete => true)
+ end
+
+ it "should retry 3 times in the case of a failure" do
+ Report.expects(:create_from_yaml).times(3).
+ raises(ActiveRecord::StatementInvalid).then.
+ raises(ActiveRecord::StatementInvalid).then.
+ returns("FINALLY!")
+
+ Report.create_from_yaml_file('/tmp/foo').should == "FINALLY!"
+ end
+
+ it "should create a DelayedJobFailure after 3 failures and return nil" do
+ Report.expects(:create_from_yaml).times(3).
+ raises(ActiveRecord::StatementInvalid).then.
+ raises(ActiveRecord::StatementInvalid).then.
+ raises(ActiveRecord::StatementInvalid).then.
+ returns("Sir not appearing in this expectation")
+
+ Report.create_from_yaml_file('/tmp/foo').should == nil
+
+ DelayedJobFailure.count.should == 1
+ DelayedJobFailure.first.summary.should == 'Importing report foo'
+ DelayedJobFailure.first.details.should ==
'ActiveRecord::StatementInvalid'
+ end
+ end
+ end
+
describe "When destroying" do
it "should destroy all dependent model objects" do
--
1.7.3.1
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
