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

Order SolidQueue failed executuions by id desc #114

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Intrepidd
Copy link
Contributor

@Intrepidd Intrepidd commented Apr 9, 2024

When inspecting the failed jobs, I want to see in an instant the most recent ones.

This seems to be the default in Sidekiq and GoodJob AFAICS.

I was very surprised to see that this was not the case with mission control + solid queue, it actually made me miss some important dead jobs that were hidden in page 2.

This PR changes the ordering of failed executions so they are ordered by (failed execution) creation ID DESC.

@@ -198,6 +198,7 @@ def order_executions(executions)
case
# Follow polling order for scheduled executions, the rest by job_id, desc or asc
when solid_queue_status.scheduled? then executions.ordered
when solid_queue_status.failed? then executions.order(created_at: :desc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a good index for this, so this won't work in general in the case you have many failed jobs 🤔 We'll need a good index first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, indeed !

happy to help on this point, but I'm not sure of the process, especially to provide migrations to end users through an update, does it necessarily have to be done through a generator like the install migrations ?

@rosa
Copy link
Member

rosa commented Apr 12, 2024

Sorry for the delay on this one, I was busy with other stuff. I kept thinking about it, though, and added a comment with an alternative idea in rails/solid_queue#207 (comment). Let me know what you think!

@Intrepidd Intrepidd changed the title Order SolidQueue failed executuions by created desc Order SolidQueue failed executuions by id desc Apr 12, 2024
@Intrepidd
Copy link
Contributor Author

@rosa I'm trying to make the specs pass, I'm having an issue on the ActiveJob::QueueAdapters::AdapterTesting::JobBatches concern.

Since it's used for both resque and solid queue (and potentially new adapters down the road) the behavior it tests has to be the same.

Right now it's (indirectly) testing the order of failed jobs, if I adapt the tests to pass for solid queue they will fail for resque, and it doesn't seem to be easy to have resque return the list of failed jobs in a descending order.

I tried to change the spec to use finished jobs instead but it seems than once the jobs are performed on resque the ActiveJob.jobs call will return an empty relationship, failing the spec.

Do you have any pointers ? Also many other specs are failing but I haven't looked at them yet

@rosa
Copy link
Member

rosa commented Apr 15, 2024

Right now it's (indirectly) testing the order of failed jobs, if I adapt the tests to pass for solid queue they will fail for resque, and it doesn't seem to be easy to have resque return the list of failed jobs in a descending order.

Ahhh, that makes sense. I think that should be doable for the Resque adapter, changing how this works. The offset would need to be applied relative to the end of the list, combined with the limit to find the new offset, something like new_offset = list_length - offset - limit - 1 (I haven't tested) and then reversing the result from Resque::Failure.all(new_offset, limit) in memory.

I understand this is a bit out of scope for this PR, although arguably, we'd have a consistent ordering across adapters 🤔

I tried to change the spec to use finished jobs instead but it seems than once the jobs are performed on resque the ActiveJob.jobs call will return an empty relationship, failing the spec.

Yeah, Resque doesn't store finished jobs anywhere, and the adapter doesn't support the finished status. Jobs are taken from the queue (which is a list in Redis) and removed from it. Only if they fail they are put back somewhere (the failed queue), but not if they finish.

I think a temporary solution could be that each adapter test could declare the order for failed jobs :desc or :asc (similarly to how they declare how to perform jobs) and then use that in the test to change the assertions.

The best solution would be to make Resque and Solid Queue behave in the same way, I think 🤔

@Intrepidd
Copy link
Contributor Author

Thanks for the pointers.

You're right, the behaviour should be the same for all adapters.

I'll try to look into it in the next days

@rosa
Copy link
Member

rosa commented Apr 15, 2024

Thank you so much! No worries if you don't have time to do this, it was supposed to be a tiny change! I'm happy to work on it myself, possibly in a week or so, as I must work on other stuff this week, but I'll get to it! 😊

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.

2 participants