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

Fix max retry count for new version of RabbitMQ x-death format #120

Merged
merged 3 commits into from
Apr 24, 2015
Merged

Fix max retry count for new version of RabbitMQ x-death format #120

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
521152e
As of March 26, Rabbitmq consolidates x-death into a single entry wit…
jaronkk Apr 20, 2015
11cdcbc
Added spec for new x-death count header
jaronkk Apr 20, 2015
13ff822
Sum the x-death to handle failures with different reasons
jaronkk Apr 21, 2015
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
9 changes: 7 additions & 2 deletions lib/sneakers/handlers/maxretry.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,14 @@ def failure_count(headers)
if headers.nil? || headers['x-death'].nil?
0
else
headers['x-death'].select do |x_death|
x_death_array = headers['x-death'].select do |x_death|
x_death['queue'] == @worker_queue_name
end.count
end
if x_death_array.count != 1
x_death_array.count
else
x_death_array.first['count'] || 1
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand, for the "retired once case", using an older version of rabbit, there will be no count attribute, and the above x_death_array count will be 1, so x_death_array.first['count'] will return nil, and so you'll return 1, does that sound about right?

Also, what happens when x_death_array is 0? Won't this raise an exception? Should there be some protection if that's the case (although it might not be something Rabbit would intentionally do).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your interpretation for the "retired once case" is correct. It might be a little more clear if the logic was refactored to something like this:

if x_death_array.count == 1 && x_death_array.first['count']
  x_death_array.first['count']
else
  x_death_array.count
end

For your second question, the x-death headers are always an array. If there isn't an entry, then it will return x_death_array.count which will be 0, which is the same thing that happens in the current version of the code. This method is returning the failure count, which will always be 0 the first time it's in the queue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, gotcha. Sorry I missed that earlier.

end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justin-yesware I updated the logic here so it's more clear when it's using the new vs legacy code. I also updated it to do a sum of the counts in case there were failures for multiple reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changes look good and the internal sum should address the issues @michaelklishin brought up.

Also, this does read a lot cleaner, thanks.

end
end
private :failure_count
Expand Down
40 changes: 40 additions & 0 deletions spec/sneakers/worker_handlers_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,31 @@ def process(*args,&block)

describe 'Maxretry' do
let(:max_retries) { nil }
let(:props_with_x_death_count) {
{
:headers => {
"x-death" => [
{
"count" => 3,
"reason" => "expired",
"queue" => "downloads-retry",
"time" => Time.now,
"exchange" => "RawMail-retry",
"routing-keys" => ["RawMail"]
},
{
"count" => 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these two counts need to be the same? It might be more illustrative to have them be separate so your test below is verifying that you're only using the count associated with the queue you're interested in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They wouldn't have to be the same. In practice the x-death count for both queues is always the same when it is returned to the worker queue, which is why I made them the same here. I could change the values if you'd prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that makes sense. I think it's better to keep the test data more realistic, so what you have here is great. Thanks for the explanation. It's been awhile since I've looked into this stuff.

"reason" => "rejected",
"queue" => "downloads",
"time" => Time.now,
"exchange" => "",
"routing-keys" => ["RawMail"]
}
]
},
:delivery_mode => 1
}
}

before(:each) do
@opts = {
Expand Down Expand Up @@ -228,6 +253,21 @@ def publish(data, opts)
Time.parse(data['failed_at']).wont_be_nil
end

it 'counts the number of attempts using the count key' do
mock(@header).routing_key { '#' }
mock(channel).acknowledge(37, false)

@error_exchange.extend MockPublish
worker.do_work(@header, props_with_x_death_count, :reject, @handler)
@error_exchange.called.must_equal(true)
@error_exchange.opts.must_equal({ :routing_key => '#' })
data = JSON.parse(@error_exchange.data)
data['error'].must_equal('reject')
data['num_attempts'].must_equal(4)
data['payload'].must_equal(Base64.encode64(:reject.to_s))
Time.parse(data['failed_at']).wont_be_nil
end

end
end

Expand Down