extracts worker.perform so call can be enhanced

[jejacks0n]

I would like to open the discussion on keyword arguments.

This pull request simply extracts the calls to worker.perform in such a way that it can be uniformly enhanced for both the processor, and the test harness. The reason for this is to allow keyword arguments to be used -- albeit with some effort, but without having to take over large methods.

I'm open to ideas, feedback, or criticism and I've only recently ventured down this path.

[jejacks0n]

Just as a follow up, the reason for this is specifically for self documenting code.

I have several interactors (interactor-rails), and almost a 1x1 mapping of sidekiq workers that call those interactors. I've designed the interactors to use named arguments because the interactor gem allows for a very loose sort of argument pattern which I don't like.

I believe I can remove 90% of the workers by including Sidekiq::Worker in my interactors -- thus allowing both perform and perform_async(etc.) to work together nicely -- while also having good self documenting code.

[mperham]

Let's see an example.

[jejacks0n]

Do you mean in the tests, or do you mean in working/psuedo form?

[jejacks0n]

class AddEmailToMailchimpList
  include Interactor
  include Sidekiq::Worker

  def perform(email:, list_id:)
    subscribe_to_list
  rescue Gibbon::MailChimpError => e
    fail!(message: e.message)
    raise e unless e.code == -100 || e.code == 214
  end
  # [...]
end

The interactor part is the precipitator of this, but I am considering the implications for Sidekiq anyway.

I'll explain how I'm using the interactor pattern just so it's clear, but it's neither here nor there directly. The way interactors work is by passing a hash to a class level perform method, which I'm so-so on because it doesn't clearly document what options can be passed. My solution was to make the perform method take keyword arguments and change how the class method invokes the instance perform method.

In addition to that I wanted to do away with many of the workers, so I threw in Sidekiq::Worker to see how easily I can use the interactor perform and sidekiq perform_async stuff together. It's mostly just a spike, but for me to be able to get something like this implemented in such a way that it's not too tied to Sidekiqs internal api, is to have a simple method to override.

[jejacks0n]

Let's talk about the implementation as well. The thing that I wasn't too thrilled about was taking the "work" method name.. it really should be more internal and not exposed in the class that includes Sidekiq::Worker, but I'm open to ideas.

[jejacks0n]

For me to do this now, I will create my own Sidekiq::InteractorWorker, with a few modifications, and in the initializers I will reopen the Sidekiq::Processor and change how the perform invocation happens via the extracted work method.

Hopefully that's enough info to convey the thought.

[mperham]

But how do you "enhance" the perform call or the work method?

[jejacks0n]

The perform invocation only happens within the work method, so:

class Sidekiq::Processor
  def self.work(worker, cloned_args)
    worker.send(:process, (cloned_args[0] || {}).symbolize_keys!)
  end
end

Not perfect, typing on my phone, but that's more or less what it would look like now. I would also do similar in the test environment to override the behavior of the test harness. The current way is to take over the whole chunk of code around the perform invocation.

It can be boiled down to "exposing more of an API". That's why I submitted the pull request without a specific test yet. We can figure out what that API looks like so I can write some tests so it can remain intact between versions, but I wanted your buy in and thoughts first.

Also, thanks for Sidekiq.

[aprescott]

For what it's worth, I've tried an alternative approach to supporting keyword args with sidekiq-symbols after previous discussions where symbols were said to not be supported in the core code.

[jejacks0n]

Thanks @aprescott. But the objective here is not to support keyword args. That's my use case, but it's to provide an API that allows a relatively easy way to override how perform is called, for any use case.

[mperham]

I'm not sold on the name work and I'm somewhat leery of private methods that some other gem depends on for implementation. Add a comment which describes why the method is necessary.

run_perform? execute_job?

[jejacks0n]

@mperham two things, not part of a gem, but something I'll likely gist about -- and I'm fine making them protected or something else. Like I said, it's really more about allowing this by simply breaking out the code that determines worker, and builds args from the part that calls it.

I like execute_job, and will change this PR to reflect that, along with providing some tests. Would you merge it if I did this?

[mperham]

Yep, sounds good.

[jejacks0n]

@mperham thanks for your time, and the library!

[mikepack]

👍 ❤️

[aprescott]

This should simplify sidekiq-symbols as a happy consequence! :)

[jejacks0n]

@aprescott awesome! that's basically what the intention is. =)

If there are smaller, more direct methods it'll make it easier to reopen/extend functionality. I only had this specific need, and there's some additional thought in how it can be abstracted so it's not part of the class methods (eg. a service object that knows how to deal only with the worker), but that's something to discuss and formulate ideas about first.

I'd be happy to take the concept a bit further in a different pull request if @mperham has ideas of how to approach that in general, but am happy to leave it as is too. =)

[jejacks0n]

@mperham wondering when this might get merged into sidekiq pro? Have been waiting on that before moving forward with a few internal adjustments.

[mperham]

Yeah, it's about time for 3.2.6. Tomorrow.

[jejacks0n]

cheers!