Skip to content

Conversation

@filipecosta90
Copy link
Contributor

@filipecosta90 filipecosta90 commented Apr 29, 2023

The following PR allows for more control of the protocol version ( avoiding errors when we know beforehand what will be the supported protocol version ) and the removal of the extra CLIENT SETNAME <string> command when we use HELLO in a successful manner.
If no ProtocolVersion is specified we default to 3. RESP3 has certain advantages since when the connection is in this mode, Redis is able to reply with more semantical replies.

@ash2k
Copy link
Contributor

ash2k commented May 3, 2023

Hi! I wonder if this will solve the problem I described here #2537? Thanks.

@monkey92t
Copy link
Collaborator

monkey92t commented May 6, 2023

I don't quite understand what problem this PR is meant to solve.

@monkey92t
Copy link
Collaborator

I suggest adding a hook to address the related issue. If the user sets a custom initialization function, we can use it instead of the default connection initialization method.

@filipecosta90 filipecosta90 changed the title Enable specifying the protocol version on connection setup. Avoid CLIENT SETNAME command when using HELLO Avoid CLIENT SETNAME command when using HELLO Jun 15, 2023
}

if c.opt.ClientName != "" {
if !clientNameSet && c.opt.ClientName != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO you can send this either way, and no need for the if check:

127.0.0.1:6379> hello 3 setname foo
1# "server" => "redis"
2# "version" => "6.2.12"
3# "proto" => (integer) 3
4# "id" => (integer) 4
5# "mode" => "standalone"
6# "role" => "master"
7# "modules" => (empty array)
127.0.0.1:6379> hello 3 setname ""
1# "server" => "redis"
2# "version" => "6.2.12"
3# "proto" => (integer) 3
4# "id" => (integer) 4
5# "mode" => "standalone"
6# "role" => "master"
7# "modules" => (empty array)
127.0.0.1:6379> 

WDYT @filipecosta90

@ndyakov
Copy link
Member

ndyakov commented Feb 20, 2025

Closing this stale PR.

@ndyakov ndyakov closed this Feb 20, 2025
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.

7 participants