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.

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

Fix miss handling empty batch fetches #376

Merged
merged 2 commits into from
Jun 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/shoryuken/manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def assign(queue, sqs_msg)
end

def dispatch_batch(queue)
batch = @fetcher.fetch(queue, BATCH_LIMIT)
return if (batch = @fetcher.fetch(queue, BATCH_LIMIT)).none?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dotpyfe ☝️ was the issue, it was trying to assign/process/delete all that stuff for empty batches.

@polling_strategy.messages_found(queue.name, batch.size)
assign(queue.name, patch_batch!(batch))
end
Expand Down
11 changes: 3 additions & 8 deletions lib/shoryuken/middleware/server/auto_delete.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,13 @@ class AutoDelete
def call(worker, queue, sqs_msg, body)
yield

auto_delete = worker.class.get_shoryuken_options['delete'] || worker.class.get_shoryuken_options['auto_delete']
return unless worker.class.auto_delete?

if auto_delete
entries = [sqs_msg].flatten.map.with_index do |message, i|
{ id: i.to_s, receipt_handle: message.receipt_handle }
end
entries = [sqs_msg].flatten.map.with_index { |message, i| { id: i.to_s, receipt_handle: message.receipt_handle } }

Shoryuken::Client.queues(queue).delete_messages(entries: entries)
end
Shoryuken::Client.queues(queue).delete_messages(entries: entries)
end
end
end
end
end

4 changes: 2 additions & 2 deletions lib/shoryuken/middleware/server/auto_extend_visibility.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ class AutoExtendVisibility
EXTEND_UPFRONT_SECONDS = 5

def call(worker, queue, sqs_msg, body)
return yield unless worker.class.auto_visibility_timeout?

if sqs_msg.is_a?(Array)
logger.warn { "Auto extend visibility isn't supported for batch workers" }
return yield
Expand Down Expand Up @@ -46,8 +48,6 @@ def auto_extend(worker, queue, sqs_msg, body)
end

def auto_visibility_timer(worker, queue, sqs_msg, body)
return unless worker.class.auto_visibility_timeout?

MessageVisibilityExtender.new.auto_extend(worker, queue, sqs_msg, body).tap(&:execute)
end
end
Expand Down
2 changes: 2 additions & 0 deletions lib/shoryuken/middleware/server/exponential_backoff_retry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ class ExponentialBackoffRetry
include Util

def call(worker, queue, sqs_msg, body)
return yield unless worker.class.exponential_backoff?

if sqs_msg.is_a?(Array)
logger.warn { "Exponential backoff isn't supported for batch workers" }
return yield
Expand Down
8 changes: 8 additions & 0 deletions lib/shoryuken/worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@ def auto_visibility_timeout?
!!get_shoryuken_options['auto_visibility_timeout']
end

def exponential_backoff?
!!get_shoryuken_options['retry_intervals']
end

def auto_delete?
!!(get_shoryuken_options['delete'] || get_shoryuken_options['auto_delete'])
end

def get_shoryuken_options # :nodoc:
@shoryuken_options || Shoryuken.default_worker_options
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def run_and_raise(worker, queue, sqs_msg, error_class)

context 'when batch worker' do
it 'yields' do
expect { |b| subject.call(nil, nil, [], nil, &b) }.to yield_control
expect { |b| subject.call(TestWorker.new, nil, [], nil, &b) }.to yield_control
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

context 'when batch worker' do
it 'yields' do
expect { |b| subject.call(nil, nil, [], nil, &b) }.to yield_control
expect { |b| subject.call(TestWorker.new, nil, [], nil, &b) }.to yield_control
end
end

Expand Down
1 change: 1 addition & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
require 'shoryuken'
require 'json'
require 'dotenv'
require 'securerandom'
Dotenv.load

if ENV['CODECLIMATE_REPO_TOKEN']
Expand Down