Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable push_bulk for various classes #3

Closed
nitzanav opened this issue Oct 7, 2016 · 3 comments
Closed

Enable push_bulk for various classes #3

nitzanav opened this issue Oct 7, 2016 · 3 comments

Comments

@nitzanav
Copy link

nitzanav commented Oct 7, 2016

Please see - 'Enable push_bulk for various classes' feature req at mperham/sidekiq repository.

I suggested there a code to enable the above mentioned feature.

Do you have any attention to push it to this repo?

@aprescott
Copy link
Owner

If I understand this right, you're proposing a method that does something like this:

[
  [FooJob, [1, 2, 3]],
  [BarJob, [10, 20, 30]]
].each do |klass, bulk_args|
  klass.push_bulk(bulk_args)
end

I think an abstraction around this looping functionality would be somewhat misleading. Without support at the Sidekiq level, pushing [[FooJob, [1, 2, 3]], [BarJob, [10, 20, 30]]] would still push in bulk on a per-class basis, not as one single Redis call for both FooJob and BarJob. There doesn't seem to be many benefits over explicitly looping over the classes that should be pushed in bulk.

If you have an application where this is a common enough pattern, you could of course write a helper method which calls push_bulk, to simplify it a little. (Although, again, it doesn't seem to give much benefit over the each.)

@nitzanav
Copy link
Author

nitzanav commented Oct 16, 2016

Hi,

It is not what I meant.

I am going to monkey patch Sidekiq::Client class to enable a real bulk_push for various worker classes that will not loop on the original bulk_push method, but will use the raw_push method and will do a single redis roundtrip for pushing tasks from various worker classes.

Here is an example of a possible monkey patch code:

# The code wasn't tested in any way, just edited here on github, so it might not even fail.
module Sidekiq
  class Client

  # The same exact current code of push_bulk, just without the raw_push
  def prepare_push_bulk_payloads(items)
    arg = items['args'].first
    return [] unless arg # no jobs to push
    raise ArgumentError, "Bulk arguments must be an Array of Arrays: [[1], [2]]" if !arg.is_a?(Array)

    normed = normalize_item(items)
    payloads = items['args'].map do |args|
      copy = normed.merge('args' => args, 'jid' => SecureRandom.hex(12), 'enqueued_at' => Time.now.to_f)
      result = process_single(items['class'], copy)
      result ? result : nil
    end.compact
  end

  # e.g. push_bulk_various_classes(['class' => WorkerA, 'args' => [['a', 1],['b', 2],['c', 3]], 'class' => WorkerB, 'args' => [[:a, 10],[:b, 20],[:c, 30]]]
  def push_bulk_various_classes(items)
    payloads = items.flat_map(&method(:prepare_push_bulk_payloads))

    raw_push(payloads) if !payloads.empty?
    payloads.collect { |payload| payload['jid'] }
  end
end

I was just suggesting for you to add this feature and monkey patch to your gem.

But you might not be interested to add this feature to your gem, and might not want to go into monkey patching.

@aprescott
Copy link
Owner

Ah, I see. Thanks for explaining. Since that involves detecting and relying on a third-party monkeypatch for compatibility between Sidekiq and other libraries, I'd prefer to avoid it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants