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

make redis schedule read an atomic pop operation via eval #107

Merged
merged 1 commit into from Jul 20, 2015

Conversation

72squared
Copy link
Contributor

This is the patch we discussed to address issue #105

@coleifer
Copy link
Owner

Cool, is there any way you could modify this to use the Script API?

https://github.com/andymccurdy/redis-py#lua-scripting

@coleifer
Copy link
Owner

I imagine the script could be registered in the __init__() method on the RedisSchedule.

@72squared
Copy link
Contributor Author

I made the suggested changes, squashing everything into one commit. Anything else?

local res = redis.call('zrangebyscore', key, '-inf', unix_ts)
if #res and redis.call('zremrangebyscore', key, '-inf', unix_ts) == #res then
return res
end
Copy link
Owner

Choose a reason for hiding this comment

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

Why is it necessary to check the return value here of zremrangebyscore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that gives you the count of the elements it removed from the sorted set. If the count doesn't match up with the number you intended, that means it wasn't able to remove those elements from the sorted set and you don't want to return those elements from the list to be worked on. It will almost never happen since redis will be working inside that single thread to do that operation atomically but still it's not a bad practice to checksum it and it won't introduce any latency.

Copy link
Owner

Choose a reason for hiding this comment

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

Cool, makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, I've been using this patch on my dev server for the past week and everything is working flawlessly.

coleifer added a commit that referenced this pull request Jul 20, 2015
make redis schedule read an atomic pop operation via eval
@coleifer coleifer merged commit 4f8e7b5 into coleifer:master Jul 20, 2015
@coleifer
Copy link
Owner

Thanks so much for making this happen!

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.

None yet

2 participants