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.

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

Avoid unnecessary retry delay in cluster client following MOVED and ASK redirection #3048

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

LINKIWI
Copy link
Contributor

@LINKIWI LINKIWI commented Jul 13, 2024

MOVED and ASK responses during command execution in a cluster mode client are not indicative of transient errors for which the client should gracefully delay before a retry. The current behavior unnecessarily constraints throughput in scenarios where:

  • The cluster topology is volatile, e.g. due to active rebalancing (Redis guarantees that keys are moved across nodes atomically during reshard)
  • The hash computation yields an incorrect slot for the command, which is at odds with the cached cluster topology state from the most recent CLUSTER SLOTS load

These effects are particularly pronounced at scale, when MinRetryBackoff is high.

This PR proposes exemption of these classes of errors during the MaxRedirects loop.

Consider the following example benchmark, which uses all default configuration options:

func BenchmarkClient(b *testing.B) {
	ctx := context.Background()
	opts := &redis.ClusterOptions{
		Addrs: []string{"localhost:7000"},
	}
	rdb := redis.NewClusterClient(opts)

	for i := 0; i < b.N; i++ {
		key := "key:" + strconv.Itoa(i)
		args := []interface{}{
			[]byte("GET"),
			[]byte(key),
		}

		err := rdb.Do(ctx, args...).Err()
		if err != nil && !errors.Is(err, redis.Nil) {
			b.Errorf(err.Error())
		}
	}
}

Current behavior, without this patch:

$ go test -bench=. -benchtime=30s ./benchmark/
goos: linux
goarch: amd64
pkg: github.com/redis/go-redis/v9/benchmark
cpu: AMD Ryzen 9 7950X 16-Core Processor            
BenchmarkClient-32    	    2787	  12479787 ns/op
PASS
ok  	github.com/redis/go-redis/v9/benchmark	36.096s

With this patch:

$ go test -bench=. -benchtime=30s ./benchmark/
goos: linux
goarch: amd64
pkg: github.com/redis/go-redis/v9/benchmark
cpu: AMD Ryzen 9 7950X 16-Core Processor            
BenchmarkClient-32    	  953277	     34109 ns/op
PASS
ok  	github.com/redis/go-redis/v9/benchmark	32.902s

@monkey92t monkey92t merged commit 5756b05 into redis:master Jul 13, 2024
10 checks passed
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.

2 participants