Delaying & Scheduling extension: Wrap ops in multi/exec #701
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.
Wrap write operations in multi/exec blocks to prevent any possibilities
of Redis being in an inconsistent state. It also has the added benefit
of providing small optimizations by reducing the number of round-trips.
The commit uses a verbose approach of wrapping most calls to
redis.multi
in aredis.pipelined
block, which is technicallyunnecessary since the redis-rb gem does not send the
multi
&exec
command if it is pipelining the commands.
The reason this commit uses both is that both methods have different
semantics,
pipelined
is meant to be an optimization whereasmulti
provides the guarantees of a Redis transaction.
While it may seem unlikely, it seems possible that future versions of
the redis gem change the underlying behavior of the
multi
andpipelined
commands. Additionally, it is a little bit more explicit interms of describing intent: "I want these commands to be run in a atomic
fashion, and I'm ok sending it all at once".
The call to
redis.del(key)
inclean_up_timestamp
was unnecessarysince the only reason that would cause the
llen
call above to return0
is if the list did not exist.The call to
pipelined
in this example might seem even more overkillsince we only give a single command to
multi
, butmulti
/exec
arethemselves commands, so in the eventuality that the redis gem would
start sending the
multi
command right away in a future version,wrapping it in a
pipelined
call is explicitly asking it to sendmulti
/zrem
/exec
all at once.I totally understand that the
redis.pipelined/redis.multi
approach I took in this commit is kinda pedantic and given that a call topipelined
is enough to guarantee atomicity, we could drop a bunch ofmulti
calls in this commit and maintain the same behavior, with the exception of the existingmulti
call inclean_up_timestamp
since none of them useWATCH
orDISCARD
.In there we could drop the
pipelined
call, and still benefit from the ruby client pipelining the commands anyway, but that would be less explicit. Happy to hear what other think