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

Enforce enqueue timestamp instead of job ids for sorting same-priority jobs #678

Closed
behrad opened this issue Jul 30, 2015 · 10 comments
Closed

Comments

@behrad
Copy link
Collaborator

behrad commented Jul 30, 2015

jobs are inserted in inactive ZSET by job.id, since http://redis.io/commands/zadd#elements-with-the-same-score, this leads to some mis-behavior in FIFO based job ordering.

@olalonde
Copy link
Contributor

olalonde commented Aug 7, 2015

Really need this to be fixed... willing to write a PR. Any pointers where to start?

@behrad
Copy link
Collaborator Author

behrad commented Aug 7, 2015

Since Kue provides a priority based queue, jobs are inserted scored by their priority.
We should look into a way of mixing scores with arrival time (created timestamp) in such a way that higher priority jobs get in front of others...

One implementation can be using the same scoring logic (priority) but inserting a composed id which contains both (timestamp + job.id), I'm not sure if it is a correct logic... but was the fastest thing came to my mind.

@olalonde
Copy link
Contributor

olalonde commented Aug 7, 2015

I'm not using priorities at the moment, all I care about is FIFO logic. Could I just set priority = -timestamp in my code?

@behrad
Copy link
Collaborator Author

behrad commented Aug 7, 2015

you can patch Kue that way, yes. But you see why I can't approve that to be merged in.

@olalonde
Copy link
Contributor

olalonde commented Aug 9, 2015

@behrad thanks. I decided to switch to RabbitMQ in the end.

@jarnosteeman
Copy link

I've ran into the same problem and fixed it in a fork. Would you be willing to accept a PR on this? It's an easy fix that basically adds the length of the id in front of it (1 => 11, 12 => 212, 123 => 3123 and so on)

@behrad
Copy link
Collaborator Author

behrad commented Aug 20, 2015

this is nice and easy fix, there existed also alike PR with Zero padding job.ids which I prefer, since you should also have the logic of splitting 3123 -> 123 which I don't know how you are doing it.
I should first see your code however I should think and compare it to that old PR, and if merged, it will be a short living merge, so it shouldn't heart any convention currently Kue user has.

We should not use job ids for any FIFO ordering logic, this will also free Kue let the user to specify their own UUIDs for jobs. This should happen for Kue 1.0

@jarnosteeman
Copy link

That would mean a fixed length on the id's though, and that length should also be pretty high so you wont run out of zero's.

About the splitting, i'm not. There would be no meaning to do so except making id sequences look 'pretty', which I personally dont care about.

@hockeytim11
Copy link
Contributor

#708
Just wanted to throw this into the discussion. I think this approach helps solve the ordering and the pretty problem.

@behrad
Copy link
Collaborator Author

behrad commented Nov 20, 2015

0.10.3

@behrad behrad closed this as completed Nov 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants