-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[redis_proxy] Add support for UNWATCH #37620
Merged
Merged
Conversation
This file contains 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
Signed-off-by: Diogo Barbosa <[email protected]>
cc @mattklein123 @weisisea shorter one this time 😄 |
dbarbosapn
changed the title
Add support for UNWATCH
[redis_proxy] Add support for UNWATCH
Dec 11, 2024
Signed-off-by: Diogo Barbosa <[email protected]>
@dbarbosapn if this is accepted, please make sure to let me know so I can update the client library's data |
Signed-off-by: Diogo Barbosa <[email protected]>
Signed-off-by: Diogo Barbosa <[email protected]>
Signed-off-by: Diogo Barbosa <[email protected]>
Signed-off-by: Diogo Barbosa <[email protected]>
Signed-off-by: Diogo Barbosa <[email protected]>
Signed-off-by: Diogo Barbosa <[email protected]>
Signed-off-by: Diogo Barbosa <[email protected]>
mattklein123
previously approved these changes
Dec 17, 2024
Sorry needs main merge. /wait |
Signed-off-by: Diogo Barbosa <[email protected]>
Done! Thank you!! 🙇 |
mattklein123
approved these changes
Dec 17, 2024
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.
This PR adds support for the
UNWATCH
command in the Redis proxy.Because of the lack of
UNWATCH
support, transactions are not well supported by clients such as StackExchange.Redis, since conditions requireUNWATCH
support.The Challenge
The challenge with implementing
UNWATCH
in Envoy is knowing which server to send theUNWATCH
command to.This was solved in this PR by considering
WATCH
a Transaction Command. The behavior is explained below.Expected Behavior
WATCH
as a transaction command.Making
WATCH
a transaction command means it will be handled by the transaction handler, not the simple request handler. There, we set the transaction key in advance, without starting the transaction. This means we will have state of the key where the latestWATCH
was sent to.UNWATCH
afterWATCH
If an
UNWATCH
is sent right after aWATCH
, we send it to the server of the transaction key that was set. After that, we reset the key. Covered by test: ✅UNWATCH
without keySending an
UNWATCH
without previous key being set, will have a direct proxy response of+OK\r\n
. Covered by test: ✅UNWATCH
afterMULTI
(no key)Sending an
UNWATCH
without a key but within a transaction, will also be a no-op in the proxy. But the response this time is+QUEUED\r\n
, to match Redis behavior. Covered by test: ✅UNWATCH
with keySending an
UNWATCH
with a transaction key set (either byWATCH
or a simple command within an active transaction) will send the command to the server of such hash key. Covered by test: ✅MULTI
afterWATCH
Since
WATCH
now sets the transaction key in advance, in this scenario we no longer queue theMULTI
command, and we send it directly upstream, based on the key set by theWATCH
command. Covered by test: ✅MULTI
afterUNWATCH
If an
UNWATCH
is sent after aWATCH
, the transaction key is reset, and theMULTI
command is queued. This is the existing behavior in the proxy. Covered by test: ✅