Skip to content
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

Fix Redis Cluster issue during roll outs of new nodes with same addr #1914

Merged
merged 3 commits into from Oct 4, 2021
Merged

Fix Redis Cluster issue during roll outs of new nodes with same addr #1914

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9375185
fix: recycle connections in some Redis Cluster scenarios
pfreixes Oct 2, 2021
129a05d
chore: fix issue with redis error detection
pfreixes Oct 4, 2021
e0d103c
chore: use switch
pfreixes Oct 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 19 additions & 4 deletions error.go
View file
Open in desktop
Expand Up @@ -65,7 +65,7 @@ func isRedisError(err error) bool {
return ok
}

func isBadConn(err error, allowTimeout bool) bool {
func isBadConn(err error, allowTimeout bool, addr string) bool {
switch err {
case nil:
return false
Expand All @@ -74,9 +74,16 @@ func isBadConn(err error, allowTimeout bool) bool {
}

if isRedisError(err) {
// Close connections in read only state in case domain addr is used
// and domain resolves to a different Redis Server. See #790.
return isReadOnlyError(err)
if isReadOnlyError(err) {
// Close connections in read only state in case domain addr is used
// and domain resolves to a different Redis Server. See #790.
return true
} else if isMovedSameConnAddr(err, addr) {
// Close connections when we are asked to move to the same addr
// of the connection. Force a DNS resolution when all connections
// of the pool are recycled
pfreixes marked this conversation as resolved.
Show resolved Hide resolved
return true
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace else if with else and add return false as a final clause. It should probably fix the tests...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a switch, hope that this also fixes the broken tests.

}

if allowTimeout {
Expand Down Expand Up @@ -119,6 +126,14 @@ func isReadOnlyError(err error) bool {
return strings.HasPrefix(err.Error(), "READONLY ")
}

func isMovedSameConnAddr(err error, addr string) bool {
redisError := err.Error()
if !strings.HasPrefix(redisError, "MOVED ") {
return false
}
return strings.HasSuffix(redisError, addr)
}

//------------------------------------------------------------------------------

type timeoutError interface {
Expand Down
2 changes: 1 addition & 1 deletion pubsub.go
View file
Open in desktop
Expand Up @@ -141,7 +141,7 @@ func (c *PubSub) releaseConn(ctx context.Context, cn *pool.Conn, err error, allo
if c.cn != cn {
return
}
if isBadConn(err, allowTimeout) {
if isBadConn(err, allowTimeout, c.opt.Addr) {
c.reconnect(ctx, err)
}
}
Expand Down
2 changes: 1 addition & 1 deletion redis.go
View file
Open in desktop
Expand Up @@ -261,7 +261,7 @@ func (c *baseClient) releaseConn(ctx context.Context, cn *pool.Conn, err error)
c.opt.Limiter.ReportResult(err)
}

if isBadConn(err, false) {
if isBadConn(err, false, c.opt.Addr) {
c.connPool.Remove(ctx, cn, err)
} else {
c.connPool.Put(ctx, cn)
Expand Down