Support connection schemes valkey:// and valkeys:// #199
Conversation
Signed-off-by: Lipeng Zhu <[email protected]>
lipzhu
changed the title
valkey://, valkeys://.
lipzhu
changed the title
valkey://, valkeys://.
|
AH!! I didn't review the tests. If there is more coming, better mark the PR as draft. :D I'll review the tests now. |
Signed-off-by: Lipeng Zhu <[email protected]>
Signed-off-by: Lipeng Zhu <[email protected]>
Signed-off-by: Lipeng Zhu <[email protected]>
|
Please also update the top comment to describe the changes (don't just link issue). also if you have a PR directly, there is actually no need to create a new issue (there is no discussion or tracking in the issue so it is actually worthless) |
| const char *scheme = "redis://"; | ||
| const char *tlsscheme = "rediss://"; | ||
| const char *scheme = "valkey://"; | ||
| const char *tlsscheme = "valkeys://"; |
There was a problem hiding this comment.
I have a concerns about this. With Redis, using rediss was clearer to me that it was SSL because there is obviously a second s. With Valkey, I'm concerned valkeys will be confusing since it's not clear. I want us to least thing about it a bit more.
There was a problem hiding this comment.
@madolson think about it for a while then. :)
Adding "s" is the most common style for URI schemes, so I think valkeys is the right choice. Here are some from IANA's list:
aaa Diameter Protocol
aaas Diameter Protocol with Secure Transport
http Hypertext Transfer Protocol
https Hypertext Transfer Protocol Secure
msrp Message Session Relay Protocol
msrps Message Session Relay Protocol Secure
rtsp Real-Time Streaming Protocol (RTSP)
rtsps Real-Time Streaming Protocol (RTSP) over TLS
sip session initiation protocol
sips secure session initiation protocol
There was a problem hiding this comment.
Ok, I thought about it and will retract my concern :) I still don't really like it, but I guess it's fine.
There was a problem hiding this comment.
I guess now would be the time to rethink the URI scheme if we wanted to do that though.
There was a problem hiding this comment.
I don't know if this scheme is widely used. It's a client feature anyway.
We can postpone it indefinitely and only support the redis scheme. Do you prefer that?
There was a problem hiding this comment.
We can consider resp as the scheme. RESP is the protocol after all. :)
There was a problem hiding this comment.
I like resp more. In fact, I think "redis" was a minomer to begin with. It should've been a protocol name.
There was a problem hiding this comment.
I think the redis scheme contains more than what's specified in RESP.
An example URL redis://user:password@host:port/dbnum contains information for the AUTH command and the SELECT command, to be called after connecting. It doesn't contain the RESP version though (for HELLO). It's a bit ad-hoc all of it.
There was a problem hiding this comment.
It could have included various more things which are useful to do when connecting, like CLIENT SETNAME, CLIENT SETINFO LIBNAME, CLIENT SETINFO LIBVER, CLIENT NO-EVICT.
For example we could support them in a query string, such as
?name=banana&libname=valkey-py&libver=3.14.159&no-evict=1
The RESP version (HELLO N) very much affects what comes out from each command, so it's basically a different protocol. We could embed that in the scheme as resp2: vs resp3:.
.... or we can just keep the redis schemes just for backward compatibility. I'm not sure we need a scheme anyway, do we?
There was a problem hiding this comment.
As long as valkey-cli can accept "redis/rediss", I am fine with other options, "valkey" or "resp2/3".
@zuiderkwast, you have a good point that the scheme implies more than just the wire protocol but also the specific client behavior so this is more of a call on the client side.
Signed-off-by: Lipeng Zhu <[email protected]>
|
I'm going to merge this. It's the least surprising schemes for any forked clients and for users moving from redis-cli to valkey-cli. We can change back if there are protests. |
zuiderkwast
changed the title
zuiderkwast
added
the
release-notes
1. Support valkey:// and valkeys:// scheme in valkey-cli and valkey-benchmark. Retain the original Redis schemes for compatibility. 2. Add unit tests for valid URI, all schemes. Fixes: valkey-io#198 Fixes: valkey-io#200 --------- Signed-off-by: Lipeng Zhu <[email protected]>
) When using a TLS scheme for valkey-cli and benchmark-cli compiled without TLS, make the error message only mention the scheme used. Before: "valkeys:// and rediss:// are only supported when valkey-cli is compiled with OpenSSL" After, depending on which scheme the user tried to use: "valkeys:// is only supported when valkey-cli is compiled with OpenSSL" "rediss:// is only supported when valkey-cli is compiled with OpenSSL" Follow up of #199. --------- Signed-off-by: karthyuom <[email protected]> Co-authored-by: k00809413 <[email protected]>
Fixes: #198
Fixes: #200