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

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

Open
skv-headless opened this issue Sep 19, 2023 · 7 comments
Open

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

skv-headless opened this issue Sep 19, 2023 · 7 comments

Comments

@skv-headless
Copy link

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
Copy link
Member

ZimbiX commented Sep 19, 2023

Hmm, I'm afraid not!

# A module that we mix into ActiveJob's wrapper for Que::Job, to maintain
# backwards-compatibility with internal changes we make.
module WrapperExtensions
module ClassMethods
# 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.
def enqueue(args, priority:, queue:, run_at: nil)
super(args, job_options: { priority: priority, queue: queue, run_at: run_at })
end
end

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

@ZimbiX
Copy link
Member

ZimbiX commented Sep 19, 2023

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
Copy link
Member

ZimbiX commented Sep 19, 2023

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 (rails/rails#44734 until rails/rails#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
Copy link
Author

skv-headless commented Sep 19, 2023

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 🙏

@skv-headless skv-headless closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2023
@ZimbiX
Copy link
Member

ZimbiX commented Sep 19, 2023

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
Copy link
Member

ZimbiX commented Sep 19, 2023

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
Copy link
Member

ZimbiX commented Sep 19, 2023

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

@ZimbiX ZimbiX reopened this Sep 19, 2023
@ZimbiX ZimbiX changed the title Is there any way to use tags with ActiveJob Doesn't support enqueueing a job with tags using ActiveJob Sep 19, 2023
ZimbiX added a commit that referenced this issue Oct 9, 2023
…equire_job_options_kwarg?

We don't need to support old versions of Que in the adapter anymore! Que's codebase always uses the latest Que =P

I considered removing & inlining Que::ActiveJob::WrapperExtensions::ClassMethods#enqueue into our Rails 7.1+ QueAdapter, but it still needs to exist in place for support for older versions of Rails =\ Keeping it in place conditionally based on the Rails version would be [a bit complex](#399 (comment)), and so perhaps not an improvement.
maxschridde1494 pushed a commit to talysto/que that referenced this issue Oct 24, 2023
…equire_job_options_kwarg?

We don't need to support old versions of Que in the adapter anymore! Que's codebase always uses the latest Que =P

I considered removing & inlining Que::ActiveJob::WrapperExtensions::ClassMethods#enqueue into our Rails 7.1+ QueAdapter, but it still needs to exist in place for support for older versions of Rails =\ Keeping it in place conditionally based on the Rails version would be [a bit complex](que-rb#399 (comment)), and so perhaps not an improvement.
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