Skip to content

Loading…

[BULK] Raise instead of just writing to stderr #224

Closed
wants to merge 2 commits into from

3 participants

@nfo
nfo commented

I happen to have a lot of documents missing, because Tire does not raise in case of failure during a bulk insertions. I think the problem is important enough to be considered as a raised exception.
In my case, my bulk insertions are triggered by Resque jobs. Without an exception, I cannot retry failed jobs.

@karmi
Owner

Hey, I think the proper way how to handle this would to allow an options hash to the bulk_store method, which would allow something like myindex.bulk_store articles, raise: true...

@nfo
nfo commented

I totally agree. It would need an update of the import method too. Do you want me to take care of this ?

@karmi
Owner

Yeah, the options would have to be passed down the chain to the import method in https://github.com/karmi/tire/blob/master/lib/tire/index.rb#L112. Definitely have a shot at that -- just make sure it's covered by unit tests, please!

@nfo nfo updated `Tire::Index#import` and `Tire::Index#bulk_store` to accept a…
… `:raise` options. This will make `bulk_store` raise after 5 tries, besides logging to STDERR.
160bb60
@karmi
Owner

@karmi @vhyza Bump!

@vhyza vhyza added a commit that closed this pull request
@nfo nfo Added the `:raise` option to Index#bulk_store and Index#import
When the `:raise` option is passed, any exception received is re-raised.

Closes #224.
a8b3e22
@vhyza vhyza closed this in a8b3e22
@karmi
Owner

@nfo Thanks, merged!

@karmi karmi added a commit that referenced this pull request
@karmi [#224] Cleaned up the Index#import interface and import/bulk_store tests
* Removed the positional argument `method` and moved it into the options Hash
* Updated tests
* Cleaned up mocked interfaces in Index tests
5363f57
@nicolas2bonfils nicolas2bonfils referenced this pull request
Commit has since been removed from the repository and is no longer available.
@nicolas2bonfils nicolas2bonfils referenced this pull request
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 2, 2012
  1. @nfo

    Raise RuntimeError if a bulk query did not succeed after 5 tries. (in…

    nfo committed
    …stead of just printing to STDERR)
  2. @nfo

    updated `Tire::Index#import` and `Tire::Index#bulk_store` to accept a…

    nfo committed
    … `:raise` options. This will make `bulk_store` raise after 5 tries, besides logging to STDERR.
This page is out of date. Refresh to see the latest.
Showing with 39 additions and 17 deletions.
  1. +4 −3 lib/tire/index.rb
  2. +31 −10 test/unit/index_test.rb
  3. +4 −4 test/unit/model_import_test.rb
View
7 lib/tire/index.rb
@@ -64,7 +64,7 @@ def store(*args)
logged([type, id].join('/'), curl)
end
- def bulk_store documents
+ def bulk_store(documents, options={})
payload = documents.map do |document|
id = get_id_from_document(document)
type = get_type_from_document(document)
@@ -92,6 +92,7 @@ def bulk_store documents
retry
else
STDERR.puts "[ERROR] Too many exceptions occured, giving up. The HTTP response was: #{error.message}"
+ raise if options[:raise]
end
ensure
@@ -109,13 +110,13 @@ def import(klass_or_collection, method=nil, options={})
documents = yield documents if block_given?
- bulk_store documents
+ bulk_store documents, options.slice(:raise)
options[:page] += 1
end
when klass_or_collection.respond_to?(:map)
documents = block_given? ? yield(klass_or_collection) : klass_or_collection
- bulk_store documents
+ bulk_store documents, options.slice(:raise)
else
raise ArgumentError, "Please pass either a collection of objects, " +
View
41 test/unit/index_test.rb
@@ -359,6 +359,14 @@ class MyDocument;end; document = MyDocument.new
assert !@index.bulk_store([ {:id => '1', :title => 'One'}, {:id => '2', :title => 'Two'} ])
end
+ should "try again and the raise when an exception occurs" do
+ Configuration.client.expects(:post).returns(mock_response('Server error', 503)).at_least(2)
+
+ assert_raise(RuntimeError) do
+ @index.bulk_store([ {:id => '1', :title => 'One'}, {:id => '2', :title => 'Two'} ], {:raise => true})
+ end
+ end
+
should "display error message when collection item does not have ID" do
Configuration.client.expects(:post).with{ |url, json| url == "#{Configuration.url}/_bulk" }.returns(mock_response('success', 200))
STDERR.expects(:puts).once
@@ -403,34 +411,47 @@ def self.count; DATA.size; end
should "just store it in bulk" do
collection = [{ :id => 1, :title => 'Article' }]
- @index.expects(:bulk_store).with( collection ).returns(true)
+ @index.expects(:bulk_store).with( collection, options={} ).returns(true)
@index.import collection
end
+ should "pass the :raise option only to bulk_store" do
+ collection = [{ :id => 1, :title => 'Article' }]
+ @index.expects(:bulk_store).with( collection, options={:raise => true} ).returns(true)
+
+ @index.import collection, method=nil, options={:raise => true, :lol => 'cats'}
+ end
+
end
context "class" do
should "call the passed method and bulk store the results" do
- @index.expects(:bulk_store).with([1, 2, 3, 4]).returns(true)
+ @index.expects(:bulk_store).with([1, 2, 3, 4], options={}).returns(true)
@index.import ImportData, :paginate
end
should "pass the params to the passed method and bulk store the results" do
- @index.expects(:bulk_store).with([1, 2]).returns(true)
- @index.expects(:bulk_store).with([3, 4]).returns(true)
+ @index.expects(:bulk_store).with([1, 2], options={}).returns(true)
+ @index.expects(:bulk_store).with([3, 4], options={}).returns(true)
@index.import ImportData, :paginate, :page => 1, :per_page => 2
end
should "pass the class when method not passed" do
- @index.expects(:bulk_store).with(ImportData).returns(true)
+ @index.expects(:bulk_store).with(ImportData, options={}).returns(true)
@index.import ImportData
end
+ should "pass the :raise option only to bulk_store" do
+ @index.expects(:bulk_store).with(ImportData, options={:raise => true}).returns(true)
+
+ @index.import ImportData, method=nil, options={:raise => true, :lol => 'cats'}
+ end
+
end
context "with passed block" do
@@ -438,7 +459,7 @@ def self.count; DATA.size; end
context "and plain collection" do
should "allow to manipulate the collection in the block" do
- Tire::Index.any_instance.expects(:bulk_store).with([{ :id => 1, :title => 'ARTICLE' }])
+ Tire::Index.any_instance.expects(:bulk_store).with([{ :id => 1, :title => 'ARTICLE' }], options={})
@index.import [{ :id => 1, :title => 'Article' }] do |articles|
@@ -451,8 +472,8 @@ def self.count; DATA.size; end
context "and object" do
should "call the passed block on every batch" do
- Tire::Index.any_instance.expects(:bulk_store).with([1, 2])
- Tire::Index.any_instance.expects(:bulk_store).with([3, 4])
+ Tire::Index.any_instance.expects(:bulk_store).with([1, 2], options={})
+ Tire::Index.any_instance.expects(:bulk_store).with([3, 4], options={})
runs = 0
@index.import ImportData, :paginate, :per_page => 2 do |documents|
@@ -465,8 +486,8 @@ def self.count; DATA.size; end
end
should "allow to manipulate the documents in passed block" do
- Tire::Index.any_instance.expects(:bulk_store).with([2, 3])
- Tire::Index.any_instance.expects(:bulk_store).with([4, 5])
+ Tire::Index.any_instance.expects(:bulk_store).with([2, 3], options={})
+ Tire::Index.any_instance.expects(:bulk_store).with([4, 5], options={})
@index.import ImportData, :paginate, :per_page => 2 do |documents|
View
8 test/unit/model_import_test.rb
@@ -39,8 +39,8 @@ class ImportTest < Test::Unit::TestCase
end
should "call the passed block on every batch, and NOT manipulate the documents array" do
- Tire::Index.any_instance.expects(:bulk_store).with([1, 2])
- Tire::Index.any_instance.expects(:bulk_store).with([3, 4])
+ Tire::Index.any_instance.expects(:bulk_store).with([1, 2], options={})
+ Tire::Index.any_instance.expects(:bulk_store).with([3, 4], options={})
runs = 0
ImportModel.import :per_page => 2 do |documents|
@@ -53,8 +53,8 @@ class ImportTest < Test::Unit::TestCase
end
should "manipulate the documents in passed block" do
- Tire::Index.any_instance.expects(:bulk_store).with([2, 3])
- Tire::Index.any_instance.expects(:bulk_store).with([4, 5])
+ Tire::Index.any_instance.expects(:bulk_store).with([2, 3], options={})
+ Tire::Index.any_instance.expects(:bulk_store).with([4, 5], options={})
ImportModel.import :per_page => 2 do |documents|
# Add 1 to every "document" and return them
Something went wrong with that request. Please try again.