Skip to content

Avoid CLIENT SETNAME command when using HELLO #2570

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

Closed
wants to merge 2 commits into from
Closed

Avoid CLIENT SETNAME command when using HELLO #2570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
959c940
Enable specifying the protocol version on connection setup. Avoid CLI…
filipecosta90 Apr 29, 2023
e0b8b64
Merge branch 'master' into protocol.version
filipecosta90 May 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@ type Options struct {

// Enables read only queries on slave/follower nodes.
readOnly bool

// Protocol Version
// Redis version 6 and above supports two protocols: the old protocol, RESP2, and a new one introduced with Redis 6, RESP3.
// Default is 3.
ProtocolVersion int
}

func (opt *Options) init() {
Expand Down Expand Up @@ -199,6 +204,13 @@ func (opt *Options) init() {
case 0:
opt.MaxRetryBackoff = 512 * time.Millisecond
}

// 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.
if opt.ProtocolVersion != 2 && opt.ProtocolVersion != 3 {
opt.ProtocolVersion = 3
}
}

func (opt *Options) clone() *Options {
Expand Down
6 changes: 4 additions & 2 deletions redis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,11 +279,13 @@ func (c *baseClient) initConn(ctx context.Context, cn *pool.Conn) error {
conn := newConn(c.opt, connPool)

var auth bool
var clientNameSet bool

// for redis-server versions that do not support the HELLO command,
// RESP2 will continue to be used.
if err := conn.Hello(ctx, 3, username, password, "").Err(); err == nil {
if err := conn.Hello(ctx, c.opt.ProtocolVersion, username, password, c.opt.ClientName).Err(); err == nil {
auth = true
clientNameSet = true
} else if !isRedisError(err) {
// When the server responds with the RESP protocol and the result is not a normal
// execution result of the HELLO command, we consider it to be an indication that
Expand Down Expand Up @@ -312,7 +314,7 @@ func (c *baseClient) initConn(ctx context.Context, cn *pool.Conn) error {
pipe.ReadOnly(ctx)
}

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

@chayim chayim Jul 2, 2023

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

pipe.ClientSetName(ctx, c.opt.ClientName)
}

Expand Down