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

[redis_proxy] Allow Multi-Key Commands in Redis Transactions #37982

Merged
merged 9 commits into from
Jan 22, 2025

Conversation

dbarbosapn
Copy link
Contributor

@dbarbosapn dbarbosapn commented Jan 13, 2025

There's no functional difference currently in allowing multi-key commands to go through in Redis transactions. The way this will work is that they're sent to the server of the first key, and this server will handle any cross-slot errors, by returning ERR MOVED or ERR CROSSSLOT.

This PR fixes this gap which will allow Envoy customers to use DEL among other important commands within transactions.

Documentation was also updated, to reflect this and past changes (a miss from my side on previous PR, sorry 😛)

Risk Level: Low
Testing: ✅
Release Notes: ✅
Docs changes: ✅

Execution sample:

image

Signed-off-by: Diogo Barbosa <[email protected]>
Signed-off-by: Diogo Barbosa <[email protected]>
Signed-off-by: Diogo Barbosa <[email protected]>
@KBaichoo
Copy link
Contributor

/assign @weisisea

as codeowner, I'm not familiar enough with redis.

@dbarbosapn this needs a main merge for changelog.

@KBaichoo KBaichoo removed their assignment Jan 15, 2025
Signed-off-by: Diogo Barbosa <[email protected]>
Signed-off-by: Diogo Barbosa <[email protected]>
@jmarantz
Copy link
Contributor

needs main merge.

@dbarbosapn re your recent force-push, please read https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md and in particular the section https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md and this bullet:

  • Once your PR is under review, please do not rebase it. If you rebase, you will need to force push to
    github, and github's user interface will force your reviewer to review the PR
    from scratch rather than simply look at your latest changes. It's much easier to review
    new commits and/or merges. We squash rebase the final merged commit so the number of commits
    you have in the PR don't matter. Again once your PR is assigned a reviewer, unless you need to fix DCO
    please do not force push. If you need to pull recent changes you can run
branch=$(git status|head -1|cut -f3 -d\ )
git checkout main
git pull
git checkout "$branch"
git merge main

/wait

@dbarbosapn
Copy link
Contributor Author

dbarbosapn commented Jan 17, 2025

needs main merge.

@dbarbosapn re your recent force-push, please read https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md and in particular the section https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md and this bullet:

  • Once your PR is under review, please do not rebase it. If you rebase, you will need to force push to
    github, and github's user interface will force your reviewer to review the PR
    from scratch rather than simply look at your latest changes. It's much easier to review
    new commits and/or merges. We squash rebase the final merged commit so the number of commits
    you have in the PR don't matter. Again once your PR is assigned a reviewer, unless you need to fix DCO
    please do not force push. If you need to pull recent changes you can run
branch=$(git status|head -1|cut -f3 -d\ )
git checkout main
git pull
git checkout "$branch"
git merge main

/wait

It was to fix DCO 😄 I fixed changelog conflicts using the GitHub UI, it said "Sign Off and Commit", commit showed verified but DCO was claiming it was not, it was red 😄 So I had to rebase unfortunately

I don't know if it's because I enabled the beta merge experience UI 🤔

Signed-off-by: Diogo Barbosa <[email protected]>
@dbarbosapn
Copy link
Contributor Author

Hey @mattklein123 , @weisisea kind ping 🙇

@mattklein123
Copy link
Member

LGTM please merge main.

/wait

Signed-off-by: Diogo Barbosa <[email protected]>
@dbarbosapn
Copy link
Contributor Author

Done. Thanks @mattklein123 🙇

@mattklein123 mattklein123 enabled auto-merge (squash) January 22, 2025 14:47
@mattklein123 mattklein123 merged commit 9a643e9 into envoyproxy:main Jan 22, 2025
24 checks passed
bazmurphy pushed a commit to bazmurphy/envoy that referenced this pull request Jan 29, 2025
…oxy#37982)

There's no functional difference currently in allowing multi-key
commands to go through in Redis transactions. The way this will work is
that they're sent to the server of the first key, and this server will
handle any cross-slot errors, by returning `ERR MOVED` or `ERR
CROSSSLOT`.

This PR fixes this gap which will allow Envoy customers to use `DEL`
among other important commands within transactions.

Documentation was also updated, to reflect this and past changes (a miss
from my side on previous PR, sorry 😛)

Risk Level: Low
Testing: ✅ 
Release Notes: ✅ 
Docs changes: ✅ 

Execution sample:


![image](https://github.com/user-attachments/assets/f63f601c-3411-4e38-9348-729f16df02b6)

---------

Signed-off-by: Diogo Barbosa <[email protected]>
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.

5 participants