Adapters

[bleonard]

Following from Issue https://github.com/taskrabbit/resque-bus/issues/10

• refactors resque into an adapter
• adds sidekiq adapter (untested)
• no longer can use separate redis from "main" resque one
• other internal refactorings

[jonsgreen]

@bleonard, one issue that I am already bumping against is that the ::Sidekiq.redis is actually a method that expects a block:

I can get around it by passing a proc to ResqueBus.redis:

  ::ResqueBus.redis { |redis| redis.hgetall(redis_key) }

and then doing something like this in the Resque Adapter:

  def redis(&block)
    block.call(::Resque.redis)
  end

A bit awkward so I am wondering if you have other ideas. Perhaps refactoring application.rb somehow?

[bleonard]

hmm, we'd have to do blocks as you mention for everyone that uses ResqueBus.redis Feel free to give that a shot if you like. Kind of a bummer. Something about the threading in Sidekiq I suppose?

[jonsgreen]

The ConnectionPool#with has a block syntax:

  def self.redis(&block)
    raise ArgumentError, "requires a block" unless block
    redis_pool.with(&block)
  end

  def self.redis_pool
    @redis ||= Sidekiq::RedisConnection.create
  end

The readme for connection_pool offers a transition Wrapper class but it is slow performance. I am not sure that is desirable unless you know those methods in application.rb are called only at startup.

[bleonard]

I can take a stab at converting it to blocks if you like.

[jonsgreen]

I am already working on it though I might need help with some specs at some point.

[bleonard]

word. thanks.

[jonsgreen]

Another thing I discovered while testing out the Sidekiq adapter is that the ResqueBus::Subscriber was not picking up the SidekiqWorker inclusion from ResqueBus::Adapters::Sidekiq#enabled! because it doesn't inherit from ResqueBus::Worker.

One way around this is to make ResqueBus::Subscriber into a class that inherits from ResqueBus::Worker and then have subscriber application classes inherit from that class rather than include as they currently do:

class MySubscriber < ResqueBusSubscriber
end

Otherwise to keep the old include DSL and be backwards compatible I can also create an adapter method that can be optionally overridden:

class MySubscriber 
  include ResqueBusSubscriber
end

module ResqueBus
  module Adapters
    class Sidekiq < ResqueBus::Adapters::Base

     def subscriber_includes(base)
        base.include ::Sidekiq::Worker
        base.include ::ResqueBus::Worker::InstanceMethods
      end
      ...
    end
  end
end

module ResqueBus
  class Worker
    # all our workers extend this one
    module InstanceMethods
      def perform(*args)
        # instance method level support for sidekiq
        self.class.perform(*args)
      end
    end
    include InstanceMethods
  end
end

module ResqueBus
  module Subscriber

    def self.included(base)
      base.extend ClassMethods
      ::ResqueBus.adapter.subscriber_includes(base)
    end
    ...
  end
end

Do you have a preference or another idea?

[jonsgreen]

Also you mentioned the idea of renaming this BackgroundBus and creating ResqueBus and SidekiqBus gems. I am wondering if and how you want to set that up. This may help deal with a few remaining aspects of the code like rake tasks, specs and gem dependencies but we could probably get around those as well.

[bleonard]

So first, it looks like I missed a perform instance method on the Subscriber mixin. I forgot that was the actual queued job. I thought it used Rider to proxy -- now that I think about it that's a better implementation... maybe later.

Assuming we keep it as-is, we need to add that instance method like in Worker

    def perform(*args)
      # instance method level support for sidekiq
      self.class.perform(*args)
    end

You can just add that which would allow Sidekiq to work as expected The Resque adapter would have to do all that business to it (adding Exponential Backoff).

I'll think about the way to do that better. Maybe the ::ResqueBus.adapter.subscriber_includes(base) pattern would be best for everyone and I could remove the ::Worker and use that for all.

[bleonard]

yes, i liked the three gem idea for a few reasons

• very clear rake setup
• clear gemspecs with real dependencies
• could more or less just copy the specs to ensure everything is working in both

For now, maybe we should have two spec folders (you can clone and rename spec to 'sidekiq_spec` or something) and different rake tasks to include into the rakefile. It would have a slightly different spec_helper for example (sets the adapter to :sidekiq)

There's some interesting issues about us migrating to the new code that I need to think through. For example, the new thing in Redis will broabbly be BackgroundBus::Rider but in transition is ResqueBus::Rider actually in Redis. It seems solvable, but I just need to think through it.

Let's get it working in this project and I'll make new repos and stuff for everything. maybe a new "organization" to hold all three (and possibly more later).

[jonsgreen]

@bleonard: I just want to make sure you saw this pull request https://github.com/taskrabbit/resque-bus/pull/14. I am not trying to put pressure; I am sure you are quite busy and have already dedicated so much effort to this project. I still need to add specs for Sidekiq integration. Just let me know where you think this is headed when you get a chance.

[bleonard]

ok, just merged your stuff into this branch. it looks good! The specs and Rakefiles and everything get so much easier if we split up the gems as discussed. I'm a bit busy today so I can't guarantee it, but I'm going to make a new base project and two adapter projects on github soon. Then you can add specs to the sidekiq one. I'll add you as a contributor on those so we can tighten up this loop a little.