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

Support ActiveJob’s wait_until #5003

Merged
merged 4 commits into from Sep 28, 2021
Merged

Support ActiveJob’s wait_until #5003

merged 4 commits into from Sep 28, 2021

Conversation

mperham
Copy link
Collaborator

@mperham mperham commented Sep 28, 2021

SomeJob.set(wait_until: 1.hour).perform_later

Sidekiq::Worker supports set to make it easy for people to convert from one API to another but does not know about wait_until. Implement it.

@mperham
Copy link
Collaborator Author

mperham commented Sep 28, 2021

We can also implement queue_as:

class MyJob
  include Sidekiq::Job
  queue_as :critical

  def perform(...)
  end
end

MyJob.set(wait_until: 1.hour).perform_async(123)

I've decided not to alias perform_async as perform_later as ActiveJob's argument serialization is very different from Sidekiq's. It would be a footgun for people to switch APIs without adjusting those call semantics.

@mperham
Copy link
Collaborator Author

mperham commented Sep 28, 2021

I have no plans to implement ActiveJob's class-level retry API or callbacks.

@mperham mperham merged commit 2b2390d into master Sep 28, 2021
@mperham mperham deleted the improve_aj_compat branch September 28, 2021 18:24
@nickgrim
Copy link

A small note, since I was confused by the first comment on this ticket: according to the ActiveJob docs, wait takes an interval (i.e. set(wait: 2.hours)) and wait_until takes a Time (i.e. set(wait_until: Time.now + 2.hours)).

Experimentally, it seems that in Sidekiq both wait and wait_until will accept either of 2.hours or 2.hours.from_now and schedule a job for "the point in time 2 hours after now", but maybe it makes sense to update any documentation / examples to be consistent with ActiveJob's usages?

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 this pull request may close these issues.

None yet

2 participants