diff --git a/clients/redis.go b/clients/redis.go index 5ed4d948..b73bb267 100644 --- a/clients/redis.go +++ b/clients/redis.go @@ -3,24 +3,25 @@ package clients import ( "context" "fmt" + "time" "github.com/contentsquare/chproxy/config" "github.com/redis/go-redis/v9" ) -func NewRedisClient(cfg config.RedisCacheConfig) (redis.UniversalClient, error) { - options := &redis.UniversalOptions{ - Addrs: cfg.Addresses, - Username: cfg.Username, - Password: cfg.Password, - PoolSize: cfg.PoolSize, - MaxRetries: 7, // default value = 3, since MinRetryBackoff = 8 msec & MinRetryBackoff = 512 msec - // the redis client will wait up to 1016 msec btw the 7 tries - } +const ( + // default value = 3, since MinRetryBackoff = 8 msec & MinRetryBackoff = 512 msec + // the redis client will wait up to 1016 msec btw the 7 tries + defaultRedisMaxRetries = 7 - if len(cfg.Addresses) == 1 { - options.DB = cfg.DBIndex - } + // go-redis keeps idle connections for 30 minutes by default, which is longer than + // the idle timeout of many firewalls/NATs (e.g. 10 minutes on GCP). Silently dropped + // connections then stall commands until they time out, so recycle them earlier. + defaultRedisConnMaxIdleTime = 5 * time.Minute +) + +func NewRedisClient(cfg config.RedisCacheConfig) (redis.UniversalClient, error) { + options := redisOptions(cfg) // maintain backwards compatibility in case of non-presence of enable_tls if len(cfg.CertFile) != 0 || len(cfg.KeyFile) != 0 || cfg.EnableTLS { @@ -41,3 +42,32 @@ func NewRedisClient(cfg config.RedisCacheConfig) (redis.UniversalClient, error) return r, nil } + +func redisOptions(cfg config.RedisCacheConfig) *redis.UniversalOptions { + options := &redis.UniversalOptions{ + Addrs: cfg.Addresses, + Username: cfg.Username, + Password: cfg.Password, + PoolSize: cfg.PoolSize, + MaxRetries: cfg.MaxRetries, + DialTimeout: time.Duration(cfg.DialTimeout), + ReadTimeout: time.Duration(cfg.ReadTimeout), + WriteTimeout: time.Duration(cfg.WriteTimeout), + PoolTimeout: time.Duration(cfg.PoolTimeout), + ConnMaxIdleTime: time.Duration(cfg.ConnMaxIdleTime), + } + + if options.MaxRetries == 0 { + options.MaxRetries = defaultRedisMaxRetries + } + + if options.ConnMaxIdleTime == 0 { + options.ConnMaxIdleTime = defaultRedisConnMaxIdleTime + } + + if len(cfg.Addresses) == 1 { + options.DB = cfg.DBIndex + } + + return options +} diff --git a/clients/redis_test.go b/clients/redis_test.go new file mode 100644 index 00000000..91709441 --- /dev/null +++ b/clients/redis_test.go @@ -0,0 +1,69 @@ +package clients + +import ( + "testing" + "time" + + "github.com/contentsquare/chproxy/config" +) + +func TestRedisOptionsDefaults(t *testing.T) { + opts := redisOptions(config.RedisCacheConfig{Addresses: []string{"127.0.0.1:6379"}}) + + if opts.MaxRetries != defaultRedisMaxRetries { + t.Errorf("expected MaxRetries %d, got %d", defaultRedisMaxRetries, opts.MaxRetries) + } + if opts.ConnMaxIdleTime != defaultRedisConnMaxIdleTime { + t.Errorf("expected ConnMaxIdleTime %s, got %s", defaultRedisConnMaxIdleTime, opts.ConnMaxIdleTime) + } + // zero values let go-redis apply its own defaults + if opts.DialTimeout != 0 || opts.ReadTimeout != 0 || opts.WriteTimeout != 0 || opts.PoolTimeout != 0 { + t.Errorf("expected zero timeouts, got dial=%s read=%s write=%s pool=%s", + opts.DialTimeout, opts.ReadTimeout, opts.WriteTimeout, opts.PoolTimeout) + } +} + +func TestRedisOptionsOverrides(t *testing.T) { + cfg := config.RedisCacheConfig{ + Addresses: []string{"127.0.0.1:6379"}, + DialTimeout: config.Duration(2 * time.Second), + ReadTimeout: config.Duration(time.Second), + WriteTimeout: config.Duration(1500 * time.Millisecond), + PoolTimeout: config.Duration(3 * time.Second), + ConnMaxIdleTime: config.Duration(4 * time.Minute), + MaxRetries: -1, + } + + opts := redisOptions(cfg) + + if opts.DialTimeout != 2*time.Second { + t.Errorf("expected DialTimeout 2s, got %s", opts.DialTimeout) + } + if opts.ReadTimeout != time.Second { + t.Errorf("expected ReadTimeout 1s, got %s", opts.ReadTimeout) + } + if opts.WriteTimeout != 1500*time.Millisecond { + t.Errorf("expected WriteTimeout 1.5s, got %s", opts.WriteTimeout) + } + if opts.PoolTimeout != 3*time.Second { + t.Errorf("expected PoolTimeout 3s, got %s", opts.PoolTimeout) + } + if opts.ConnMaxIdleTime != 4*time.Minute { + t.Errorf("expected ConnMaxIdleTime 4m, got %s", opts.ConnMaxIdleTime) + } + if opts.MaxRetries != -1 { + t.Errorf("expected MaxRetries -1, got %d", opts.MaxRetries) + } +} + +func TestRedisOptionsDBIndexOnlyForSingleAddress(t *testing.T) { + single := redisOptions(config.RedisCacheConfig{Addresses: []string{"a:6379"}, DBIndex: 3}) + if single.DB != 3 { + t.Errorf("expected DB 3 for a single address, got %d", single.DB) + } + + cluster := redisOptions(config.RedisCacheConfig{Addresses: []string{"a:6379", "b:6379"}, DBIndex: 3}) + if cluster.DB != 0 { + t.Errorf("expected DB 0 for multiple addresses, got %d", cluster.DB) + } +} diff --git a/config/README.md b/config/README.md index 45abf23f..56b11508 100644 --- a/config/README.md +++ b/config/README.md @@ -122,6 +122,17 @@ redis: password: pool_size: db_index: | default = 0 [optional] # This option is only applicable for non-clustered Redis instance. + # Timeouts of the redis client. When omitted, the go-redis defaults apply. + dial_timeout: | default = 5s [optional] + read_timeout: | default = 3s [optional] + write_timeout: | default = read_timeout [optional] + pool_timeout: | default = read_timeout + 1s [optional] # max time to wait for a free connection from the pool + # Max time a pooled connection may stay idle before being closed. + # Keep it below the idle timeout of any firewall/NAT between chproxy and redis + # (e.g. 10m on GCP), otherwise commands may stall on silently dropped connections. + conn_max_idle_time: | default = 5m [optional] + # Max number of retries of a failed command. -1 disables retries. + max_retries: | default = 7 [optional] # Expiration time for cached responses. expire: diff --git a/config/config.go b/config/config.go index a8f94c4e..508090d3 100644 --- a/config/config.go +++ b/config/config.go @@ -966,12 +966,25 @@ type RedisCacheConfig struct { TLS `yaml:",inline"` EnableTLS bool `yaml:"enable_tls,omitempty"` - Username string `yaml:"username,omitempty"` - Password string `yaml:"password,omitempty"` - Addresses []string `yaml:"addresses"` - DBIndex int `yaml:"db_index,omitempty"` - PoolSize int `yaml:"pool_size,omitempty"` - XXX map[string]interface{} `yaml:",inline"` + Username string `yaml:"username,omitempty"` + Password string `yaml:"password,omitempty"` + Addresses []string `yaml:"addresses"` + DBIndex int `yaml:"db_index,omitempty"` + PoolSize int `yaml:"pool_size,omitempty"` + + // Timeouts of the redis client. Zero values fall back to the go-redis defaults. + DialTimeout Duration `yaml:"dial_timeout,omitempty"` + ReadTimeout Duration `yaml:"read_timeout,omitempty"` + WriteTimeout Duration `yaml:"write_timeout,omitempty"` + PoolTimeout Duration `yaml:"pool_timeout,omitempty"` + + // ConnMaxIdleTime is the max time a pooled connection may stay idle before being closed. + ConnMaxIdleTime Duration `yaml:"conn_max_idle_time,omitempty"` + + // MaxRetries is the max number of retries of a failed command. -1 disables retries. + MaxRetries int `yaml:"max_retries,omitempty"` + + XXX map[string]interface{} `yaml:",inline"` } // UnmarshalYAML implements the yaml.Unmarshaler interface. diff --git a/config/config_test.go b/config/config_test.go index 76c1e233..adf44e16 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -50,10 +50,16 @@ var fullConfig = Config{ MaxPayloadSize: ByteSize(100 << 30), SharedWithAllUsers: true, Redis: RedisCacheConfig{ - Username: "chproxy", - Password: "password", - Addresses: []string{"127.0.0.1:" + redisPort}, - PoolSize: 10, + Username: "chproxy", + Password: "password", + Addresses: []string{"127.0.0.1:" + redisPort}, + PoolSize: 10, + DialTimeout: Duration(2 * time.Second), + ReadTimeout: Duration(time.Second), + WriteTimeout: Duration(time.Second), + PoolTimeout: Duration(2 * time.Second), + ConnMaxIdleTime: Duration(4 * time.Minute), + MaxRetries: 2, }, }, }, @@ -926,6 +932,12 @@ caches: addresses: - 127.0.0.1:%s pool_size: 10 + dial_timeout: 2s + read_timeout: 1s + write_timeout: 1s + pool_timeout: 2s + conn_max_idle_time: 4m + max_retries: 2 max_payload_size: 107374182400 shared_with_all_users: true param_groups: diff --git a/config/testdata/full.yml b/config/testdata/full.yml index 020a5d9f..7ea25ca3 100644 --- a/config/testdata/full.yml +++ b/config/testdata/full.yml @@ -61,6 +61,19 @@ caches: username: chproxy password: password pool_size: 10 + # Optional client timeouts. Zero values fall back to the defaults of the + # underlying go-redis client. + dial_timeout: 2s + read_timeout: 1s + write_timeout: 1s + pool_timeout: 2s + # Max idle time of a pooled connection before it's closed. Keep it below + # any idle timeout of firewalls/NATs between chproxy and redis. + # By default 5m. + conn_max_idle_time: 4m + # Max number of retries of a failed command. -1 disables retries. + # By default 7. + max_retries: 2 addresses: - 127.0.0.1:16379 max_payload_size: 107374182400