Merged
Conversation
Random command like SPOP with count is replicated as some SREM operations, and store them in also_propagate array to propagate after the call, but this would break atomicity. To keep the command's atomicity, wrap also_propagate array with MULTI/EXEC.
in case of nested MULTI/EXEC
Contributor
|
Thanks @soloestoy, quick question before reviewing it: during the creation of this PR, did you also check that the modules propagation APIs will play well with the change? Since now the modules APIs used alsoPropagate() and used to send the EXEC after the context was destroyed. |
Contributor
Author
Contributor
|
Thanks @soloestoy, PR merged. |
Contributor
|
To be honest I don't remember if the reason why BRPOPLPUSH was propagated as two commands was because of the expire. Probably yes? Commit is too old, and there was no clear description about why it was propagated in that way. |
JackieXie168
pushed a commit
to JackieXie168/redis
that referenced
this pull request
Dec 24, 2019
…ulti Wrap also propagate as multi
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
2 participants
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.
Hi @antirez @madolson , now I open this PR to discuss the codes I mentioned in #6152 , except the original problem, here are other three changes:
propagate
EXECdirectly in lua scriptI think we don't need to use also propagate in lua, cause
MULTIis already propagated.flag module client as
CLIENT_MULTIif neededIn case of nested MULTI/EXEC, lua script has the same problem, see details in scripting: flag lua_client as CLIENT_MULTI after redis.replicate_command() immediately #5780
propagte
BRPOPLPUSHasRPOPLPUSHwhen unblockAfter the expire problem is fixed, I think it's OK to do that now
BTW, if this PR is OK, before merge it we should merge #5780 at first to avoid nested MULIT/EXEC when
SPOPwith count is called in lua script.