8000 Disallowing Client Reply is On / Off / Skip when Client is Multi by sarthakaggarwal97 · Pull Request #1966 · valkey-io/valkey · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Disallowing Client Reply is On / Off / Skip when Client is Multi #1966

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: unstable
Choose a base branch
from
Open

Disallowing Client Reply is On / Off / Skip when Client is Multi #1966

wants to merge 5 commits into from

Conversation

sarthakaggarwal97
Copy link
Contributor
@sarthakaggarwal97 sarthakaggarwal97 commented Apr 17, 2025 •
edited by zuiderkwast
Loading

Return an error when we try to perform CLIENT REPLY command under MULTI/EXEC.

Without this change, if CLIENT REPLY is used in a transaction, it causes EXEC to return broken RESP protocol, with a multi-bulk length mismatching the actual number of multi-bulk elements.

Resolves #1268

@sarthakaggarwal97 sarthakaggarwal97 marked this pull request as ready for review April 17, 2025 02:49
@codecov Codecov
Copy link
codecov bot commented Apr 17, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.97%. Comparing base (70f2057) to head (d4ccb1a).
Report is 1 commits behind head on unstable.

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #1966      +/-   ##
============================================
- Coverage     70.97%   70.97%   -0.01%     
============================================
  Files           123      123              
  Lines         66135    66137       +2     
============================================
  Hits          46941    46941              
- Misses        19194    19196       +2
Files with missing lines Coverage Δ
src/server.c 87.92% <100.00%> (-0.03%) ⬇️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

madolson
madolson reviewed Apr 21, 2025
View reviewed changes
madolson
madolson reviewed Apr 24, 2025
View reviewed changes
@sarthakaggarwal97 sarthakaggarwal97 changed the title Disallowing Multi when Client Reply is On / Off / Skip Disallowing Client Reply is On / Off / Skip when Client is Multi Apr 24, 2025
@sarthakaggarwal97
Copy link
Contributor Author

@madolson this should require a doc change as well right?

@madolson madolson added release-notes This issue should get a line item in the release notes breaking-change Indicates a possible backwards incompatible change labels Apr 25, 2025
@madolson
Copy link
Member

Future note for those that are looking at this. This is a breaking change, but the client protocol is already broken by this change. @valkey-io/core-team just a ping to see if anything has changed since our previous conversations. I'll merge this if I don't hear back.

@zuiderkwast
Copy link
Contributor

High-level approved by me.

I didn't immediately understand the problem so I read the issue. Then I edited the top comment to explain what was broken. Please correct it if I got it wrong.

@madolson
Copy link
Member

@sarthakaggarwal97 Yeah, we should probably update the docs, good point.

@madolson madolson added the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Apr 28, 2025
hpatro
hpatro approved these changes Apr 29, 2025
View reviewed changes
@sarthakaggarwal97
disallowing multi when client reply is on / off / skip
7937d8d 
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
@sarthakaggarwal97
addressing comment
8689640 
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
@sarthakaggarwal97
fixing test comment
c5f3e6a 
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
@sarthakaggarwal97
reordering the assertions
600f700 
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
zuiderkwast
zuiderkwast reviewed Apr 29, 2025
View reviewed changes
@sarthakaggarwal97
comments
d4ccb1a 
Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
@sarthakaggarwal97 sarthakaggarwal97 mentioned this pull request Apr 29, 2025
Open
@sarthakaggarwal97
Copy link
Contributor Author

@madolson I have raised a doc PR as well. Let me know if everything looks good, thank you!

@madolson madolson removed the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zuiderkwast zuiderkwast zuiderkwast left review comments

@hpatro hpatro hpatro approved these changes

@madolson madolson madolson approved these changes

@enjoy-binbin enjoy-binbin Awaiting requested review from enjoy-binbin

Assignees
No one assigned
Labels
breaking-change Indicates a possible backwards incompatible change release-notes This issue should get a line item in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] CLIENT REPLY OFF in transaction handled incorrectly
4 participants
@sarthakaggarwal97 @madolson @zuiderkwast @hpatro
0