Fix max_retries Method #31
Conversation
| @@ -13,10 +13,17 @@ def worker_dog_options(worker) | |||
| end | |||
|
|
|||
| def max_retries(worker) | |||
| retries = worker.class.get_sidekiq_options['retry'] || Sidekiq[:max_retries] | |||
There was a problem hiding this comment.
This was causing the worker to always default to the global Sidekiq[:max_retries] value even ifworker.class.get_sidekiq_options['retry'] was set to false, which is NOT the intended behavior.
lib/sidekiq/instrument/mixin.rb
Outdated
| case retries.to_s | ||
| when "true" | ||
| Sidekiq[:max_retries] | ||
| when "false" | ||
| 0 | ||
| when "" | ||
| Sidekiq[:max_retries] | ||
| else | ||
| retries | ||
| end |
There was a problem hiding this comment.
Consider
| case retries.to_s | |
| when "true" | |
| Sidekiq[:max_retries] | |
| when "false" | |
| 0 | |
| when "" | |
| Sidekiq[:max_retries] | |
| else | |
| retries | |
| end | |
| case retries.to_s | |
| when "true", "" | |
| Sidekiq[:max_retries] | |
| when "false" | |
| 0 | |
| else | |
| retries | |
| end |
There was a problem hiding this comment.
But, honestly, we should not be passing in boolean values at all, Sidekiq is clearly written with numbers in mind (and just doesn't validate that)
There was a problem hiding this comment.
Oh neat, I didn't know that syntax was allowed in Ruby - good shout thanks.
I agree that it's unfortunate we have to handle so many cases because there doesn't really seem to be a good standardized way we're doing it :(
There was a problem hiding this comment.
As a best practice, when describing a context, start its description with 'when', 'with' or 'without'.
Currently
max_retriesis not properly calculating a value of 0 when the retry value is set tofalse. This change fixes themax_retriesmethod to accurately determine the max retry count in all cases. It also adds thorough testing to cover these cases.