search

brandonhilkert / sucker_punch

Sucker Punch is a Ruby asynchronous processing library using concurrent-ruby, heavily influenced by Sidekiq and girl_friday.
MIT License
2.65k stars 114 forks source link

Adding queue options to Job so that you can set the max_queue value. #218

Closed seandmccarthy closed 6 years ago

seandmccarthy commented 6 years ago

We were evaluating Sucker Punch and found that we wanted to restrict the size of the queue for jobs instead of having unlimited as the normal setting.

I've tried to follow the style used for options already in Job, while remaining backward compatible.

If there's a reason for fixing the queue size to unlimited, feel free to reject this PR.

Otherwise happy to make any changes requested if you'd like to add this feature.

brandonhilkert commented 6 years ago

@seandmccarthy Hey! Makes sense. My only concern with the proposed implementation is that the max_queue is in the queue_options, while workers is a class method.

In hindsight, I should've put max_workers in something like queue_options. I'm concerned that users will attempt to put workers in queue_options and it won't have the effect they want. Because of this, I'd like to propose we move max_queue to a class method like workers.

One other thought, while the name max_queue is the equivalent for concurrent-ruby, stepping back, I'm unsure if the name properly reflects how users would think of it. I'm not totally convinced of this, but wanted to at least explore other names that might be clearer. My first thought was max_jobs, job_limit, queue_limit. What do you think of those names? Think any one is more user-friendly for someone well-versed in concurrency?

seandmccarthy commented 6 years ago

Checking I understand correctly we effectively want to replace "queue_opts" and "queue_options" in the Job class with "max_queue". That makes sense since it's the only "queue option" so far.

I like your 3 suggestions for alternative names to max_queue. Since the option is on the Job class, maybe the "max_jobs" name fits best.

seandmccarthy commented 6 years ago

Hi @brandonhilkert, I've finally had a chance to update this PR with your suggestions. If you have any other feedback I'm happy to update accordingly. Thanks.

seandmccarthy commented 6 years ago

Hi @brandonhilkert, changes made as requested. Any other refinements I can make?

brandonhilkert commented 6 years ago

Last, add something to the change log describing the feature with a sample and we'll merge it up and ship version 2.1.0. Thanks!

brandonhilkert commented 6 years ago

@seandmccarthy Thanks so much for sticking through it!

brandonhilkert commented 6 years ago

Version 2.1.0 has been released!