Allow sidekiq_delay and not just delay

[neojin]

Hello,

Great gem, love it. Today I was having trouble with async calls to the subscriber and I noticed that the jobs are sent to sidekiq via delay. This is problematic if you use delayed_jobs and sidekiq in the same project. A workaround to this is to use sidekiq_delay, which you can turn on via Sidekiq.hook_rails!

https://github.com/mperham/sidekiq/wiki/Delayed-extensions#disabling-extensions

I'm not sure if there is a better way to do this (maybe there is an internal setting in Sidekiq that can be checked), but just wanted to present one way to fix it.

If you wanted to accept this version, I can also write a quick test for it.

[coveralls]

Coverage remained the same at 100.0% when pulling 3564a6c on neojin:master into fc5a529 on krisleech:master.

[krisleech]

I'd be happy to accept a PR which checks for sidekiq_delay first and uses it, otherwise it uses delay 👍

[krisleech]

Please could you add a test, then I can merge. Thanks.

[neojin]

Ok great, will do once I get off work. It should probably belong in a separate model_spec so I will create a new spec file if that is alright with you.

[krisleech]

No problem :)

[movstox]

@krisleech : I get Do not call .delay on a Sidekiq::Worker class, call .perform_async error when using the gem with the latest sidekiq:

[PROJECT_ROOT]/vendor/bundle/ruby/2.2.0/gems/sidekiq-4.0.0/lib/sidekiq/worker.rb:42→ delay
[PROJECT_ROOT]/vendor/bundle/ruby/2.2.0/gems/wisper-sidekiq-0.0.1/lib/wisper/sidekiq.rb:9→ broadcast
[PROJECT_ROOT]/vendor/bundle/ruby/2.2.0/gems/wisper-1.6.1/lib/wisper/registration/object.rb:16→ broadcast
[PROJECT_ROOT]/vendor/bundle/ruby/2.2.0/gems/wisper-1.6.1/lib/wisper/publisher.rb:65→ block in broadcast
[PROJECT_ROOT]/vendor/ruby-2.2.3/lib/ruby/2.2.0/set.rb:283→ each_key
[PROJECT_ROOT]/vendor/ruby-2.2.3/lib/ruby/2.2.0/set.rb:283→ each
[PROJECT_ROOT]/vendor/bundle/ruby/2.2.0/gems/wisper-1.6.1/lib/wisper/publisher.rb:64→ broadcast

should i submit a separate PR?

[krisleech]

A PR would be great. It needs to be backwards compatible, so check for #perform_async first and use, otherwise fall back to #delay. Maybe check for #sidekiq_delay too as per the original issue.

I think the order should be [:perform_async, :sidekiq_delay, :delay].

[movstox]

ok, i'll give it a try. I'm pretty new to this :)

On 18.11.2015 at 09:06 GMT None wrote:

A PR would be great. It needs to be backwards compatible, so check for #perform_async first and use, otherwise fall back to #delay. Maybe check for #sidekiq_delay too as per the original issue.

I think the order should be [:perform_async, :sidekiq_delay, :delay].

—
Reply to this email directly or #5 (comment)<>.

[movstox]

@krisleech it seems that the problem is bigger:

just replacing delay with perform_async won't work, since perform_async expects a Job to have just one method perform. On the other side, a Listener can wait for multiple events to arrive thus having just perform method won't be sufficient...

Any thoughts on what could be the best solution in this case?

[krisleech]

@movstox I'm happy to help.

It doesn't sound like #perform_async is a direct replacement for #delay then. The Wiki suggest you can still use #delay (https://github.com/mperham/sidekiq/wiki/Delayed-extensions).

One thing I will ask - does your listener include Sidekiq::Worker? If so, it shouldn't. I think this might be problem :)

[movstox]

@krisleech take a look at this movstox@a0b34a5

It fixes spec tests, but I doubt it's backward compatible.

[krisleech]

The code looks fine but I think it fixes a problem you don't have, see previous comment :)

[krisleech]

That is unless you want your listener to also be a Sidekiq worker as you'll also use it outside of Wisper...

[movstox]

@krisleech https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/worker.rb#L42 raises an exception if I run spec on your current master ...
Wiki may be outdated though. My goal is to use a Listener in async mode with the latest Sidekiq.

[krisleech]

Did you you remove the include Sidekiq::Worker from your listener as suggested?

[krisleech]

The Sidekiq master branch still has delay: https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/extensions/class_methods.rb#L21

I think the error you are getting is because Wisper is calling delay on a SideKiq worker, the listener shouldn't be a Sidekiq worker, just a plain Ruby class.

[movstox]

@krisleech I see now, sorry for confusion and thanks for your help!

[krisleech]

No problem :) Did it solve you problem, if so, can you close the issue please.

[krisleech]

(sorry I see it isn't your issue)