Dont take into account context canceled and rate limit errors in redi…

…s limiter. (#309)

* Add some tests to redis limiter.
* Dont take into account context canceled in limiter.
* Add option to set min number of idle connections to redis.
* Prevent limiter from counting its own errors towards rate limit.

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.

VERSION

@@ -1 +1 @@
-11.10.1
+11.10.2

redis/BUILD

@@ -16,11 +16,28 @@ 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",
         "///third_party/go/github.com_peterebden_go-cli-init_v4//flags",
         "///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",
+    ],
+)

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()
 	}
 }

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())
+			}
+		})
+	}
+
+}

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 {