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

Status::Hash: Reduce redis round-trips #139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pjambet
Copy link

@pjambet pjambet commented Nov 12, 2020

The calls to zadd & zremrangebyscore should be consistent, that is,
we don't one want to succeed and one to fail. On top of that, we can
send both at once to prevent one roundtrip.

In set, the call to expire was unnecessary, we can use set or
setex instead, or, ideally, if we were ok with supporting only redis
2.6.12+, we could switch to set with the ex option.

Finally, in remove, we can also wrap del and zrem for the same
reasons we wrapped the two calls in create.

The use of pipelined and multi is a bit verbose, and technically
unnecessary since the ruby gem drops the multi/exec calls within a
pipelined call, and a multi call by itself uses pipelining, but doing it
as such is more explicit wrt intent:

"We want to wrap these in a transaction for the sake of atomicity, and
we might as well optimize the network calls, and let Redis be faster as
well, by pipelining the whole thing"

The calls to `zadd` & `zremrangebyscore` should be consistent, that is,
we don't one want to succeed and one to fail. On top of that, we can
send both at once to prevent one roundtrip.

In `set`, the call to `expire` was unnecessary, we can use `set` or
`setex` instead, or, ideally, if we were ok with supporting only redis
2.6.12+, we could switch to `set` with the `ex` option.

Finally, in `remove`, we can also wrap `del` and `zrem` for the same
reasons we wrapped the two calls in `create`.

The use of `pipelined` and `multi` is a bit verbose, and technically
unnecessary since the ruby gem drops the multi/exec calls within a
pipelined call, and a multi call by itself uses pipelining, but doing it
as such is more explicit wrt intent:

"We want to wrap these in a transaction for the sake of atomicity, and
we might as well optimize the network calls, and let Redis be faster as
well, by pipelining the whole thing"
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.

1 participant