-
Notifications
You must be signed in to change notification settings - Fork 645
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
Add command "Client Capa subv2" to change behavior for SUBSCRIBE and SSUBSCRIBE commands: one call with only one response #962
base: unstable
Are you sure you want to change the base?
Conversation
cbdafa8
to
c4fff33
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #962 +/- ##
============================================
+ Coverage 70.61% 70.67% +0.05%
============================================
Files 114 114
Lines 61740 61763 +23
============================================
+ Hits 43599 43648 +49
+ Misses 18141 18115 -26
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a design error from the beginning, and another design error when RESP3 was introduced, but it is too late to change it now.
This is a serious breaking change that will break all clients that support pubsub. This is very dangerous to do, considering there are many redis-compatible services out there and clients try to be compatible with not only valkey.
if (c->resp == 2) | ||
addReply(c, shared.mbulkhdr[number]); | ||
else | ||
addReplyPushLen(c, number); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with RESP3, this is still no normal in-band replies and one push message (instead of one per channel). This is still the main problem of [UN][P|S]SUBSCRIBE
IMO.
To fix this behaviour, SUBSCRIBE should return an in-band reply. Either the list of channels as an array or map-reply, or just +OK and send the channels as push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not want to import this change in Valkey 8.0. I wish we could do this change on Valkey 8.X or 9 version. Error need to be correct, never late ^_^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we cannot fix it ever. Only maybe with a CLIENT CAPA or RESP4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with this being a potentially significant breaking change. I think maybe CLIENT CAPA could fix it (or RESP4).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I update the design, add a new parameter for client capa command, but the parameter name is just temporarily. If you have better name, please let me know or we can discuss it in the meeting
c4fff33
to
3af8b90
Compare
30403a0
to
8bae942
Compare
8bae942
to
79f7352
Compare
79f7352
to
f7ed3e2
Compare
@hwware What exactly is the problem with the old behaviour according to you? According to the docs, when a client enters subscribed mode, the protocol changes semantics from a request-response protocol to a push protocol. The commands and the responses are no longer related to each other. This was for RESP2 and many commands are forbidden in subscribed mode to make sure the client can understand the responses. It is a limited number of possible responses. The client can receive pubsub messages and notifications that a channel is subscribed or unsubscribed, asynchronously, but not exactly as a reply to each command. In RESP3, it is different, because here clients can call any command in subscribed mode. The reason for allowing this was that RESP3 has a push (out-of-band) type. The client just needs to know the very strange fact that SUBSCRIBE and UNSUBSCRIBE receive no in-band reply at all. Now, there are two things that make this more complicated:
Most of this is possible for clients to handle, even if it is very complex to handle error replies in subscribed mode, but one problem is not possible to solve. This one: #1066. I have another idea that can solve the issue of matching request-response pairs:
|
f7ed3e2
to
c97985f
Compare
Hi @zuiderkwast As what you said, the current behavior is that the protocol changes semantics from a request-response protocol to a push protocol if the client enters subscribed mode. The commands and the responses are no longer related to each other. Thus, the client could receive multiply responses from one command. It results the client side (such as Java side, or Javascript etc) need to check which mode or protocol the server current supports now. It causes the client side programming more complex. Thus, in my PR, I think we should unify the request-response protocol and push protocol response way , make the response always matches with the command (1:1). We should try our best to decrease the client programming complexity. (Or by my pr, give the client one option) |
OK, but there are responses that can't be matched with a command, the messages sent by PUBLISH:
(And additionally the spontaneous That's why in RESP2 can never be 1:1 request-response protocol in pubsub. We need an out-of-band message for this, which we have in RESP3. If we just use it correctly, pubsub can be a 1:1 where only in-band replies are matched with a command and push messages are out-of-band. Clients that want better semantics can use RESP3. |
CLIENT CAPA is not enough, because the client needs to know if the server supports the feature too. The client can check the response of HELLO to see the Valkey version. Can the client trust the version number to infer the subscribe behaviour? Alternatives:
|
c97985f
to
a0c0586
Compare
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
a0c0586
to
d7117c1
Compare
Now for the command SUBSCRIBE and SSUBSCRIBE, if any client call with multiply channels, multiply responses come back.
It does not match with the Valkey command response logic, it causes client side to parse the response with extra effort on them. The problem exists long time.
Thus, to fix this design error, I add a new command parameter: client capa subv2
The command "client capa" is imported in Valkey 8, and I add a new parameter "subv2" for it
Note: subv2 is just a temporarily name, I have no idea which name is better, welcome any suggestion.
The update behavior as below: one call with one response
Note: for existing users, I think this is a break change. If any client chooses the behavior that one call with one response, the command "client capa subv2" can be called first.