diff --git a/ChangeLog b/ChangeLog index 61f83b23..027ac3c8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +Version 11.10.2 +-------------- + * Add some tests to redis limiter. + * Don't take into account context canceled in limiter. + * Add option to set mininum number of idle connections to redis. + * Prevent limiter from counting its own errors towards rate limit. + Version 11.10.1 -------------- * Stop printing warnings when we get redis.Nil errors. diff --git a/VERSION b/VERSION index 46577f54..bd0f144b 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -11.10.1 +11.10.2 diff --git a/redis/BUILD b/redis/BUILD index 0f9c1d02..faa69884 100644 --- a/redis/BUILD +++ b/redis/BUILD @@ -16,7 +16,10 @@ sh_cmd( go_library( name = "redis", - srcs = glob(["*.go"]), + srcs = glob( + ["*.go"], + exclude = ["*_test.go"], + ), visibility = ["PUBLIC"], deps = [ "///third_party/go/github.com_go-redis_redis_v8//:v8", @@ -24,3 +27,17 @@ go_library( "///third_party/go/golang.org_x_time//rate", ], ) + +go_test( + name = "redis_test", + srcs = glob( + ["*_test.go"], + ), + deps = [ + ":redis", + "///third_party/go/github.com_go-redis_redis_v8//:v8", + "///third_party/go/github.com_stretchr_testify//assert", + "///third_party/go/github.com_stretchr_testify//require", + "///third_party/go/golang.org_x_time//rate", + ], +) diff --git a/redis/limiter.go b/redis/limiter.go index 9e72362d..73153c73 100644 --- a/redis/limiter.go +++ b/redis/limiter.go @@ -1,12 +1,16 @@ package redis import ( + "context" "errors" "github.com/go-redis/redis/v8" "golang.org/x/time/rate" ) +// ErrRateLimitReached is returned when the rate limit of errors is reached. +var ErrRateLimitReached = errors.New("limiter error rate exceeded, skipping Redis call") + // Limiter implements redis.Limiter so we can rate limit calls to Redis if // they fail too often. type Limiter struct { @@ -17,14 +21,17 @@ type Limiter struct { // we won't allow the redis call. func (l *Limiter) Allow() error { if l.limiter.Tokens() < 1.0 { - return errors.New("limiter error rate exceeded, skipping Redis call") + return ErrRateLimitReached } return nil } // ReportResult records any non-nil error against the rate limiter. func (l *Limiter) ReportResult(err error) { - if err != nil && err != redis.Nil { + if err != nil && + !errors.Is(err, redis.Nil) && + !errors.Is(err, ErrRateLimitReached) && + !errors.Is(err, context.Canceled) { l.limiter.Reserve() } } diff --git a/redis/limiter_test.go b/redis/limiter_test.go new file mode 100644 index 00000000..bc72233e --- /dev/null +++ b/redis/limiter_test.go @@ -0,0 +1,83 @@ +package redis + +import ( + "context" + "errors" + "testing" + + "github.com/go-redis/redis/v8" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/time/rate" +) + +func TestLimiterAllow(t *testing.T) { + tests := map[string]struct { + l *Limiter + + expectedError bool + }{ + "returns true if rate limiter has some tokens left": { + l: &Limiter{limiter: rate.NewLimiter(0.1, 1)}, + expectedError: false, + }, + "returns false if rate limiter has no token left": { + l: &Limiter{limiter: &rate.Limiter{}}, + expectedError: true, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + err := test.l.Allow() + if test.expectedError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestLimiterReportResult(t *testing.T) { + tests := map[string]struct { + result error + + allowMore bool + }{ + "allows nil errors": { + allowMore: true, + }, + "allows redis.Nil errors": { + result: redis.Nil, + allowMore: true, + }, + "allows context canceled errors": { + result: context.Canceled, + allowMore: true, + }, + "allows rate limit errors": { + result: ErrRateLimitReached, + allowMore: true, + }, + "other errors are not allowed and count towards rate limit": { + result: errors.New("random infra error"), + allowMore: false, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + // Allows one error every 10 seconds + l := &Limiter{limiter: rate.NewLimiter(0.1, 1)} + require.NoError(t, l.Allow()) + l.ReportResult(test.result) + if test.allowMore { + assert.NoError(t, l.Allow()) + } else { + assert.Error(t, l.Allow()) + } + }) + } + +} diff --git a/redis/options.go b/redis/options.go index 0f0f0a7a..ea90920e 100644 --- a/redis/options.go +++ b/redis/options.go @@ -23,19 +23,21 @@ const DefaultMaxSize int64 = 200 * 1012 // 200 Kelly-Bootle standard units // logic that calls the clients. This is just so we have a single place where // this option is defined. type Opts struct { - URL string `long:"url" env:"REDIS_URL" description:"host:port of Redis server"` - ReadURL string `long:"read_url" env:"REDIS_READ_URL" description:"host:port of a Redis read replica, if set any read operation will be routed to it"` - Password string `long:"password" description:"AUTH password"` - PasswordFile string `long:"password_file" env:"REDIS_PASSWORD_FILE" description:"File containing AUTH password"` - PoolSize int `long:"pool_size" env:"REDIS_POOL_SIZE" default:"10" description:"Size of connection pool on primary redis client"` - ReadPoolSize int `long:"read_pool_size" env:"REDIS_READ_POOL_SIZE" default:"10" description:"Size of connection pool on reading redis client"` - PoolTimeout flags.Duration `long:"pool_timeout" env:"REDIS_POOL_TIMEOUT" default:"5s" description:"Timeout waiting for free connection to primary redis"` - ReadPoolTimeout flags.Duration `long:"read_pool_timeout" env:"REDIS_READ_POOL_TIMEOUT" default:"5s" description:"Timeout waiting for free connection to read replicas"` - ReadTimeout flags.Duration `long:"read_timeout" env:"REDIS_READ_TIMEOUT" default:"1s" description:"Timeout on network read (not read commands)"` - WriteTimeout flags.Duration `long:"write_timeout" env:"REDIS_WRITE_TIMEOUT" default:"1m" description:"Timeout on network write (not write commands)"` - CAFile string `long:"ca_file" env:"REDIS_CA_FILE" description:"File containing the Redis instance CA cert"` - TLS bool `long:"tls" description:"Use TLS for connecting to Redis"` - MaxSize int64 `long:"max_size" env:"REDIS_MAX_SIZE" default:"202400" description:"Max size of objects indexed on redis"` // default is 200 Kelly-Bootle standard units + URL string `long:"url" env:"REDIS_URL" description:"host:port of Redis server"` + ReadURL string `long:"read_url" env:"REDIS_READ_URL" description:"host:port of a Redis read replica, if set any read operation will be routed to it"` + Password string `long:"password" description:"AUTH password"` + PasswordFile string `long:"password_file" env:"REDIS_PASSWORD_FILE" description:"File containing AUTH password"` + PoolSize int `long:"pool_size" env:"REDIS_POOL_SIZE" default:"10" description:"Size of connection pool on primary redis client"` + ReadPoolSize int `long:"read_pool_size" env:"REDIS_READ_POOL_SIZE" default:"10" description:"Size of connection pool on reading redis client"` + MinIdleConns int `long:"min_idle_conns" env:"REDIS_MIN_IDLE_CONNS" default:"0" description:"Minimum number of idle connections to primary redis."` + ReadMinIdleConns int `long:"read_min_idle_conns" env:"REDIS_READ_MIN_IDLE_CONNS" default:"0" description:"Minimum number of idle connections to read replicas."` + PoolTimeout flags.Duration `long:"pool_timeout" env:"REDIS_POOL_TIMEOUT" default:"5s" description:"Timeout waiting for free connection to primary redis"` + ReadPoolTimeout flags.Duration `long:"read_pool_timeout" env:"REDIS_READ_POOL_TIMEOUT" default:"5s" description:"Timeout waiting for free connection to read replicas"` + ReadTimeout flags.Duration `long:"read_timeout" env:"REDIS_READ_TIMEOUT" default:"1s" description:"Timeout on network read (not read commands)"` + WriteTimeout flags.Duration `long:"write_timeout" env:"REDIS_WRITE_TIMEOUT" default:"1m" description:"Timeout on network write (not write commands)"` + CAFile string `long:"ca_file" env:"REDIS_CA_FILE" description:"File containing the Redis instance CA cert"` + TLS bool `long:"tls" description:"Use TLS for connecting to Redis"` + MaxSize int64 `long:"max_size" env:"REDIS_MAX_SIZE" default:"202400" description:"Max size of objects indexed on redis"` // default is 200 Kelly-Bootle standard units } // Clients sets up clients to both primary and read replicas. If no read URL @@ -63,6 +65,7 @@ func (r Opts) Clients() (primary, read *redis.Client) { ReadTimeout: time.Duration(r.ReadTimeout), WriteTimeout: time.Duration(r.WriteTimeout), PoolTimeout: time.Duration(r.PoolTimeout), + MinIdleConns: r.MinIdleConns, Limiter: limiter, }) if r.ReadURL != "" { @@ -74,6 +77,7 @@ func (r Opts) Clients() (primary, read *redis.Client) { ReadTimeout: time.Duration(r.ReadTimeout), WriteTimeout: time.Duration(r.WriteTimeout), PoolTimeout: time.Duration(r.ReadPoolTimeout), + MinIdleConns: r.ReadMinIdleConns, Limiter: limiter, }) } else {