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

Run lock timeout configurable #164

Merged
merged 3 commits into from
Feb 17, 2016

Conversation

Slania
Copy link

@Slania Slania commented Feb 15, 2016

class ConfigurableRunLockTimeoutWorker
  include Sidekiq::Worker
  sidekiq_options :queue => 'background', :run_lock_expiration => 60 * 3
end

@Slania
Copy link
Author

Slania commented Feb 15, 2016

We use sidekiq jobs to do things like send out emails through slow APIs. With batching on our end, their end, XML processing etc., it is actually very common for some of our workers to take a couple of minutes or more to send out a large batch (~4000) of emails.

Right now, the run_lock is hardcoded to expire in 60seconds unless the lease is renewed. This PR makes it possible to configure the expiration time.

@Slania Slania force-pushed the configurable_run_lock_timeout branch from 15df161 to dc60832 Compare February 15, 2016 13:27
@Slania
Copy link
Author

Slania commented Feb 16, 2016

@mhenrixon thoughts?

@@ -3,7 +3,7 @@ class SimpleWorker
sidekiq_options unique: :until_executed,
unique_args: -> (args) { [args.first] },
queue: :default,
default_expiration: 3 * 60 * 60
default_queue_lock_expiration: 3 * 60 * 60
Copy link
Owner

Choose a reason for hiding this comment

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

Still thinking about this but maybe default_queue_expiration and default_run_expiration would suffice. We already know it is expiration of a lock so it feels redundant. On the other hand it is very explicit. What do you think?

@mhenrixon
Copy link
Owner

Good job, had some feedback for you @Slania. Good thinking with the naming of the lock_expiration and the default_lock_expiration. 👍

@Slania
Copy link
Author

Slania commented Feb 17, 2016

@mhenrixon I think I have it. I used the same file for all 3 calculators just because it made sense to keep them close together and the file itself is relatively small. However, they each have their own specs though. Inheritance did make the testing a lot easier and more terse! :)

@Slania Slania force-pushed the configurable_run_lock_timeout branch 2 times, most recently from f093669 to c5f2f7a Compare February 17, 2016 16:52
@Slania Slania force-pushed the configurable_run_lock_timeout branch from c5f2f7a to a2799d5 Compare February 17, 2016 18:51
mhenrixon added a commit that referenced this pull request Feb 17, 2016
@mhenrixon mhenrixon merged commit ccac479 into mhenrixon:master Feb 17, 2016
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.

2 participants