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

Allow sidekiq_retry_in to return a range #4957

Closed
wants to merge 1 commit into from

Conversation

maurycy
Copy link

@maurycy maurycy commented Aug 7, 2021

The idea is to make it easier to implement (and reason about) exponential (and not only!) backoffs with jitter.

@maurycy maurycy changed the title retry_in range Allow sidekiq_retry_in to return a range Aug 7, 2021
@mperham mperham closed this Aug 7, 2021
@maurycy
Copy link
Author

maurycy commented Aug 7, 2021

@mperham Any reason why? It would make it easier to reason about exponential backoffs with jitter. (Thank you for your work on sidekiq!)

@mperham
Copy link
Collaborator

mperham commented Aug 7, 2021

“No description provided.”

if you make no effort to explain your PR to others, I make no effort to review it.

@maurycy
Copy link
Author

maurycy commented Aug 7, 2021

@mperham Updated. I appreciate the fast feedback, but sometimes it's too fast. ;-) You closed it in 5 minutes while I was still pondering the best description. Please let me know if you want me to expand on this description.

@mperham mperham reopened this Aug 7, 2021
@mperham
Copy link
Collaborator

mperham commented Aug 7, 2021

👍🏻

@mperham
Copy link
Collaborator

mperham commented Aug 7, 2021

I appreciate the feedback. In the future I’ll give people a while to finalize the PR before taking any actions.

@mperham
Copy link
Collaborator

mperham commented Aug 7, 2021

What happens if the block returns nil? 0?

@maurycy
Copy link
Author

maurycy commented Aug 7, 2021

Yes:

irb(main):003:0> nil.is_a?(Range)
=> false
irb(main):004:0> nil.to_i
=> 0

hence to_i:

https://github.com/mperham/sidekiq/pull/4957/files#diff-a2a78bd5586e1300341bb91dc8be382aa98126dcde11bd507fe9886d2e643ee7R223
https://github.com/mperham/sidekiq/pull/4957/files#diff-a2a78bd5586e1300341bb91dc8be382aa98126dcde11bd507fe9886d2e643ee7L218

hence > 0:

https://github.com/mperham/sidekiq/pull/4957/files#diff-a2a78bd5586e1300341bb91dc8be382aa98126dcde11bd507fe9886d2e643ee7L219

If you mean an endless range then it raises an exception:

irb(main):023:0> rand((1..nil))
Traceback (most recent call last):
        6: from /Users/maurycy/.rbenv/versions/2.6.5/bin/irb:23:in `<main>'
        5: from /Users/maurycy/.rbenv/versions/2.6.5/bin/irb:23:in `load'
        4: from /Users/maurycy/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        3: from (irb):23
        2: from (irb):23:in `rescue in irb_binding'
        1: from (irb):23:in `rand'
Errno::EDOM (Numerical argument out of domain)

I'm not sure what is the best behaviour here.

@maurycy
Copy link
Author

maurycy commented Aug 8, 2021

Other reasons why I think this change makes sense:

  • random retries should be encouraged, even seconds_to_delay already uses rand,
  • sidekiq_retry_in is always a bit probabilistic, because there is no way to guarantee a retry exactly at a given time.

@mperham
Copy link
Collaborator

mperham commented Aug 8, 2021 via email

@maurycy
Copy link
Author

maurycy commented Aug 8, 2021

Why not both?

Some users might want to retry within a short period, others prefer a longer one, and range seems like the easiest and the most intuitive way to configure this, without breaking anything and ready to go immediately.

Moving in the direction of building in explicit randomness needs some consideration: which jitter algorithm? leave the current seconds_to_delay formula as is and focus only on the sidekiq_retry_in block? what API – how to let users configure the interval etc.?

I like the direction, though. What do you think?

@mperham
Copy link
Collaborator

mperham commented Aug 9, 2021

I strongly believe software needs less knobs, not more. I'd prefer the user return a single Integer and we add jitter internally. That's how Sidekiq's retry subsystem works today. Is the root issue here "sidekiq_retry_in should include jitter so lots of jobs don't pile up at the exact same time"?

@maurycy
Copy link
Author

maurycy commented Aug 9, 2021

I strongly believe software needs less knobs, not more. I'd prefer the user return a single Integer and we add jitter internally.

There's still a need for configuration I think.

Is the root issue here "sidekiq_retry_in should include jitter so lots of jobs don't pile up at the exact same time"?

Yes.

Another idea: perform_in accepting a range. ;-)

@mperham
Copy link
Collaborator

mperham commented Aug 9, 2021

Ok, I think the simpler solution is to add jitter to the return value from sidekiq_retry_in. perform_in can't add jitter as people really depend on that timing but retries are fair game and already do.

@mperham mperham closed this in 3575ccb Aug 9, 2021
@maurycy maurycy deleted the retry_in_range branch August 9, 2021 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants