que-rb / que

A Ruby job queue that uses PostgreSQL's advisory locks for speed and reliability.
MIT License
2.31k stars 188 forks source link

Doesn't support enqueueing a job with tags using ActiveJob #399

Open skv-headless opened 1 year ago

skv-headless commented 1 year ago

Tags work: ChargeCreditCard.enqueue(card.id, user_id: current_user.id, job_options: { tags: ["hello"] })

Tags don't work: ChargeCreditCard.perform_later(card.id, user_id: current_user.id, job_options: { tags: ["hello"] })

Maybe I miss something?

ZimbiX commented 1 year ago

Hmm, I'm afraid not!

https://github.com/que-rb/que/blob/9744019261eccc4bd3a2f3d05d45c957632c2d22/lib/que/active_job/extensions.rb#L62-L70

I'll have a quick look into how tags support could be added.

ZimbiX commented 1 year ago

I've not used tags before, nor have I ever been clear on their purpose. Is there a reason you can't have your tags be a job kwarg?

ZimbiX commented 1 year ago

We've dropped support for job options supplied as top-level keywords, but ActiveJob's QueAdapter still uses them. So we have to move them into the job_options hash ourselves.

I looked into this. The manual construction of job_options was only in edge Rails for 6 months (https://github.com/rails/rails/pull/44734 until https://github.com/rails/rails/pull/46005 / 2022-03-22 – 2022-09-13), with both merge commits showing a Git tag of v7.1.0.beta1, so given that, I read that it never made it into the v7.1.0 release. However, it was backported to 7-0-stable, and it's still present there, so I gathered that it was released in a v7.0.x, and yes, it was released in v7.0.4.

So. We can't just assume it's safe to change the handling of job_options in Que's ActiveJob extensions.

skv-headless commented 1 year ago

I've not used tags before, nor have I ever been clear on their purpose.

for example synchronization produces some amount of jobs. I mark them with tag and get synchronization progress.

Is there a reason you can't have your tags be a job kwarg

I think I can. It would just require some refactoring

Wanted your opinion before starting to refactor. Thank you for a quick response 🙏

ZimbiX commented 1 year ago

We could add tags as another top-level kwarg. It would be consistent to do that, given the other job_options are configured at that level - priority, queue, and run_at (although run_at is set via wait_until... or it was, but maybe that mapping's been lost now the Que adapter's been removed from ActiveJob... =S ). But that could be a breaking change for systems using Que that have a job kwarg called tags.

ZimbiX commented 1 year ago

Another option that would allow you to keep using tags would be to insert that job using Que.enqueue. To keep compatibility with ActiveJob, just ensure you're doing it such that the resulting row inserted into the que_jobs table is the same as it would've been if it were enqueued using ActiveJob (other than the tags).

ZimbiX commented 1 year ago

I'm going to reopen and rename the issue so it can remain tracked