Be able to access headers in work() about sneakers HOT 15 CLOSED

Hi Benjamin,

The headers were hidden from the developer in order to avoid exposing AMQP internals, so that a developer will work in the mind frame of what he/she already knows with libraries like Sidekiq, Resque and delayed_job.

I think it should not be a problem to expose them - however I would like to preserve simplicity. I would be happy to hear what kind of example API you would expect that would serve you best and still be simple.

Thanks

from sneakers.

Hi Dotan, that was quick ;-)

As the AMQP guide state its good design to use the AMQP metadata headers (http://rubybunny.info/articles/exchanges.html#message_metadata). Usually i keep the user_id and other related information to the actual data in there in the headers key.
So far this would be easily accessible in the work method, for simplicity i'd just hand it through for now making it a second parameter.

def work(msg, headers) # not hdr
  ...

The higher up message metadata (routing_key, reply_to, ..) are very useful also sometimes. but it would complicate the access to the headers (2 levels).

To keep the interface nice would keep those out of the work() method and instead add a nice interface to make use of them: For example reply() to publish a message to the supplied reply_to key.
For this to be possible we would need a task-class wrapping the work() method to keep the information and state.
Adding this would break the current design a bit so i'm not quite sure if you want to go that way.

Kinda like this:

def do_work(hdr, props, msg, handler)
  ...
  # some 'magic' to determine the class
  task = SomeTask.new(hdr, props, msg)
  task.call
  res = task.state
  ...
end

class Sneakers::Task
  def initialize(hdr, props, msg)
    @state = :new 
    @props = props
    @headers = props[:headers]
    ...
  end

  def ack!
    @state = :ack
  end

  def reply(msg)
    publish(msg, @props[:reply_to])
  end
  ...
end

class SomeTask
  include Sneakers::Task

  def call
     # this would replace the work() in Worker
     # do some work
     reply('some result')
     ack!
  end
end

This is pretty much how i usually handle this part, and i was just about to start hacking it into sneakers so i can test it a bit with existing Tasks

from sneakers.

Since i anyway tried it, here is a (running, minimal) example:

maxigs@12b40e7

for this code

class SomeTasksWorker
  include Sneakers::Worker
  from_queue :some_tasks

  class Task
    include Sneakers::Task
    def call
      ack!
    end
  end
end

from sneakers.

wow, thanks! I will look into it once I have the time to experiment. I know this might be risking premature optimization - but with the original idea I tried not to create "holder" objects in order to reduce GC overhead, this is why I didn't have the "task" abstraction. However this looks so clean I'm wondering if we can offer both now.
In any case I hope to get into it once I have a good chunk of clean time coming up :)

from sneakers.

We could just make it optional, like so:

if self.respond_to?(:work)
  work(..)
elsif defined?((self.class.name + '::Task').constantize)
  klass = (self.class.name + '::Task').constantize
  task = klass.new(...)
  task.call
  ...
else
  raise 'no work() method or task class'
end

That would not break the old pattern, but allow for the new one to just add more functionality.

from sneakers.

The API i use now, with a bit less magic:

class SomeWorker
  include Sneakers::Worker
  from_queue :tasks, {
    ...
    task_class:        SomeWorkerTask
  }
end

class SomeWorkerTask
  def call
    ...
    ack!
  end
end

Rails did not really like the nested classes (autoload magic), and i think explicitly putting it as a setting is nicer anyway if there is more options.

The change in worker.rb is also really straight forward for this:

res = if @opts[:task_class]
  task = @opts[:task_class].new(msg: msg,...)
  task.call
  task.status
else
  work(msg)
end

While it allows to add whatever additional data to the Task instance. Sorry its not a pull request, but its so simple i think its ok ;)

from sneakers.

Hey Benjamin, sorry for the blackout, had some personal matters. Trying to allocate time to work on Sneakers issues now 👍

from sneakers.

So I gave it some research, and I may be wrong, and I wouldn't be surprise if I am, but generating more objects would generate a GC overhead we can do away with.
How about taking your initial suggestion for detecting a method early on, and then using it instead?

Let's say something like this

class MyWorker
  include Sneakers::Worker
  from_queue :foo

  def work_with_params(msg, headers, params)
  end
end

Then, Sneakers will have to detect which methods the end-user implemented via your respond_to? suggestion (but only once, on initialization time) and cache the result.
All future calls will go to work_with_params.

What do you think?

from sneakers.

you're welcome to test the new branch, there's working tests for example of a worker and usage :)

from sneakers.

Didn't test it yet but the interface looks fine. If needed everyone can still initialise a task class within the work_with_params method in his code as long as that one as access to it.

I will definitely keep using "task-classes" as they make testing the code much easier.

from sneakers.

Thanks, Benjamin, I agree your way of doing things is more encapsulated and cleaner code. I've merged the with_params variant and hope it can support both your use case and others.
Pushed as v0.1.0.pre for early adopters. Install with gem install sneakers --pre

from sneakers.

👍 Thanks for this :)

from sneakers.

@jondot is this interface still available? I came across this when googling sneakers and trying to access headers

from sneakers.

@alilland something like this?

from sneakers.

@michaelklishin this is perfect, solves half my issue, how can I access headers within my def work(msg); end method?

I am desperately trying to find a way to track how many times a message is requeued

from sneakers.