sidekiq-cron / sidekiq-cron

Scheduler / Cron for Sidekiq jobs
MIT License
1.85k stars 279 forks source link

[ActiveJob] Cannot use `queue_as` for select queue, only sidekiq options are supported (first run) #437

Closed arathunku closed 8 months ago

arathunku commented 10 months ago

Hi :wave:

looks like sidekiq-cron works "okayish" with ActiveJob. It detects and runs jobs:)

There are 2 problems I'm running into

  1. here only sidekiq options are fetched on init, if sidekiq options change later on, this will not be fetched
  2. When some of the options change, for example queue, it will be only overwritten if passed by args see here.

No code here supports ActiveJob configuration.

Is this more of a bug and PRs to get better ActiveJob is welcome or there's something wrong I'm doing?

Repro is very minimal

  foobar:
    cron: "*/5 * * * *" 
    class: "FooBar"
  class FooBar < ApplicationJob
    queue_as :low

    def perform
markets commented 10 months ago

Hey @arathunku 👋

In #424 we are discussing some changes to properly use AJ. Do you think those changes can help with this issue too?

arathunku commented 10 months ago

I've seen the issue but decided to open a new issue because I found no need for any global options nor active_job: true. Detection of ActiveJob jobs by is_active_job? works for enquing jobs and so far, I've only found the two missing parts documented above.

I prefer to see options on "job" level as much as possible, where you'd usually make a change and follow the same pattern both for non-cron-job Jobs and normal background jobs.

For CronJobs, we also set up additional defaults via:

  class SetupCronJobDefaults
    def self.setup_for_all
      ConfigLoader.load("sidekiq-cron.yml").each do |(_name, job)|
        klass = Sidekiq::Cron::Support.constantize(job.fetch("class", job["klass"]))
         .....
      end
    end
  end

This is executed before config is loaded.

So that nobody needs to remember about some incantation for every [cron] job definition.

markets commented 10 months ago

Ok @arathunku! yeah that seems quite different to "global" defaults. I'm happy to review PRs to make it easy to use under AJ. Feel free to send a proposal.