Delaying & Scheduling extension: Wrap ops in multi/exec#701
Open
pjambet wants to merge 1 commit intoresque:masterfrom
Open
Delaying & Scheduling extension: Wrap ops in multi/exec#701pjambet wants to merge 1 commit intoresque:masterfrom
pjambet wants to merge 1 commit intoresque:masterfrom
Conversation
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 a `redis.pipelined` block, which is _technically_ unnecessary 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 whereas `multi` 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` and `pipelined` commands. Additionally, it is a little bit more explicit in terms 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)` in `clean_up_timestamp` was unnecessary since the only reason that would cause the `llen` call above to return `0` is if the list did not exist. The call to `pipelined` in this example might seem even more overkill since we only give a single command to `multi`, but `multi`/`exec` are themselves 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 send `multi`/`zrem`/`exec` all at once.
Author
|
Tests failed for ruby 2.6 and resque ~1.27, trying to replicate locally, need to install the right ruby version and all that Update: Tests pass on my machine with ruby 2.6.6 and resque 1.27.4, which confirms my gut feeling that the failures are not directly related to my changes. Happy to hear if there's anything else I can do. Could it be that these tests randomly fail? Or is the test suite usually pretty robust and does not have random failures? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.multiin aredis.pipelinedblock, which is technicallyunnecessary since the redis-rb gem does not send the
multi&execcommand if it is pipelining the commands.
The reason this commit uses both is that both methods have different
semantics,
pipelinedis meant to be an optimization whereasmultiprovides 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
multiandpipelinedcommands. 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_timestampwas unnecessarysince the only reason that would cause the
llencall above to return0is if the list did not exist.The call to
pipelinedin this example might seem even more overkillsince we only give a single command to
multi, butmulti/execarethemselves commands, so in the eventuality that the redis gem would
start sending the
multicommand right away in a future version,wrapping it in a
pipelinedcall is explicitly asking it to sendmulti/zrem/execall at once.I totally understand that the
redis.pipelined/redis.multiapproach I took in this commit is kinda pedantic and given that a call topipelinedis enough to guarantee atomicity, we could drop a bunch ofmulticalls in this commit and maintain the same behavior, with the exception of the existingmulticall inclean_up_timestampsince none of them useWATCHorDISCARD.In there we could drop the
pipelinedcall, and still benefit from the ruby client pipelining the commands anyway, but that would be less explicit. Happy to hear what other think