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

Pull into Sidekiq core? #6

Closed
mperham opened this issue Apr 25, 2024 · 6 comments · Fixed by sidekiq/sidekiq#6286
Closed

Pull into Sidekiq core? #6

mperham opened this issue Apr 25, 2024 · 6 comments · Fixed by sidekiq/sidekiq#6286

Comments

@mperham
Copy link

mperham commented Apr 25, 2024

Hey @fatkodima, would you be interested in integrating this functionality into Sidekiq core for 7.3 or have me do it? I've had several customers report this gem as very useful for solving their problems with long-running jobs, making deployments quicker and safer, etc. I think it's a good pattern/API to encourage people to use.

@fatkodima
Copy link
Owner

Hey! Wow, thats awesome to get this merged into sidekiq itself!

I will try to do that on this weekend (or next weekend) and see how it goes. Let me know if you have plans to release 7.3 sooner.

@mperham
Copy link
Author

mperham commented Apr 25, 2024

I have a 7.3 milestone targeting a summer release. 7.2.3 will be out very soon.

@fatkodima
Copy link
Owner

Wanted to ask, what API would you prefer?

  1. (my preference)
class MyJob
  include Sidekiq::Job
  include Sidekiq::Iteration
end

or something like
2.

class MyJob
  include Sidekiq::Job
  sidekiq_options iteration: true, ...
end

And what API would you prefer for throttling (https://github.com/fatkodima/sidekiq-iteration/blob/master/guides/throttling.md)? Currently it is configured via a top level call in the class' body.

@mperham
Copy link
Author

mperham commented May 2, 2024

I'd probably go with:

class SomeJob
  include Sidekiq::Job
  include Sidekiq::Job::Iterable

  sidekiq_options iteration: { whatever: 123 }
end

Unlike Rails, I dislike top-level class methods like throttle_on as they can be hard to test and mock. I would prefer that be an instance method, server middleware provides an instance:

class ThrottleMiddleware
  include Sidekiq::ServerMiddleware

  def call(instance, job, queue)
    if instance.throttle_on?
      # do something
    end
  end
end

@sobrinho
Copy link

sobrinho commented May 7, 2024

As suggestion @mperham, I feel like the framework should be pulled into Sidekiq but not the concrete implementations.

AR can be suggested to be used as I reported on #9:

def build_enumerator(cursor:)
  Enumerator.new do |yielder|
    MyModel.in_batches(start: cursor) do |relation|
      yielder.yield(relation, relation.maximum(:id))
    end
  end
end

def each_iteration(relation)
  relation.update_all(...)
end

Or for batches:

def build_enumerator(cursor:)
  Enumerator.new do |yielder|
    MyModel.find_in_batches(start: cursor) do |batch|
      yielder.yield(batch, batch.last.id)
    end
  end
end

def each_iteration(batch)
  batch.each { ... }
end

Or for individual records:

def build_enumerator(cursor:)
  Enumerator.new do |yielder|
    MyModel.find_each(start: cursor) do |record|
      yielder.yield(record, record.id)
    end
  end
end

def each_iteration(record)
  record.update(...)
end

Feels like having the CSV, Array and AR may be too much, I'm not sure, just throwing ideas out here.

@mperham
Copy link
Author

mperham commented May 7, 2024

Having optimized support for a few well known types/libraries is useful but we should have generic Enumerable support too.

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

Successfully merging a pull request may close this issue.

3 participants