Implement a solid queue adapter #23
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Just for skipping tests for now, while I add pause support to Solid Queue.
Everything is implemented except for pausing/resuming, which is still not supported.
rosa
force-pushed
the
solid-queue-adapter
branch
from
127ee2a
to
5869f07
Compare
rosa
force-pushed
the
solid-queue-adapter
branch
from
5869f07
to
d5888de
Compare
Probably should do this in the house-style repo, though.
It doesn't do anything, as there's nothing to change there. In the future, if solid queue accept different DBs and we have different adapters, perhaps we could do something here to select the database.
jorgemanrubia
approved these changes
Sep 16, 2023
rosa
commented
Sep 16, 2023
| @@ -186,7 +190,7 @@ def fetch_failed_resque_jobs | |||
|
|
|||
| def fetch_queue_resque_jobs | |||
| unless jobs_relation.queue_name.present? | |||
| raise ActiveJob::Errors::QueryError, "This adapter only supports fetching failed jobs when no queue name is provided" | |||
| raise ActiveJob::Errors::QueryError, "This adapter requires a queue name unless fetching failed jobs" | |||
There was a problem hiding this comment.
I changed this because I found it, personally, a bit confusing. I thought it meant that you must not provide a queue name when fetching failed jobs, but it's actually that if you don't provide a queue name, the only thing you can do is fetching failed jobs. I hope you don't mind!
Before this worked without issues because resque was the only adapter set, but after introducing Solid Queue, depending on the order of the tests, it might happen that we run a resque test after a solid queue test, that will finish with the solid queue adapter being used. This is a problem if we try to fetch jobs of a certain kind via Active Job (eg. ApplicationJob.jobs.pending and things like that), as that would use Solid Queue.
…eue_adapter` This mimics what our job server does. It's not strictly necessary because Resque keeps a global reference to the Redis connection being used, and activating just that is enough even if another adapter is being used, but I find this clearer.
rosa
force-pushed
the
solid-queue-adapter
branch
from
55daa1e
to
b428928
Compare
This messes up with failed filtering, and prevents it from working in Solid Queue. In Solid Queue, failed is not a queue but a status of jobs, that continue to belong to other queues. In Resque, failed is a separate queue, so when we use the default queue when querying failed jobs, that's ignored and it doesn't matter. However, for Solid Queue, we end up showing failed jobs in the default queue alone, which is not what we want, since right now we don't have a way to filter failed jobs by queue. We want to show all jobs in all queues with failed status. In all the other cases where we filter jobs, we want to provide a queue explicitly, not the default one, and this is what happens everywhere. Not having a default saves us from having to explicitly unset the queue every time we want to fetch failed jobs.
This has changed in Ruby 3.3, from 3 arguments to 2, and it's not public API, so it makes sense that it broke.
rosa
force-pushed
the
solid-queue-adapter
branch
from
b428928
to
2ec4f7a
Compare
rosa
force-pushed
the
solid-queue-adapter
branch
from
7f4fb35
to
5eb1b0f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Besides the adapter itself with tests for it (massive kudos, @jorgemanrubia, for the existing testing harness for adapters you built, that was incredibly useful!),
I've included a tiny change that I'll remove soon: allow adapters to declare that they don't support pausing queues, just for skipping tests for now, while I add pause support to Solid Queue. Resque has support via a plugin but I haven't built this yet. Should be easy, in any case, so much that I don't think it's worth to account for this in the UI.Related work supporting this PR:
inlinerunning mode for tests.