From 36ed5dd61914a7c0fbd96d65f794efb2e67eb340 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Tue, 2 Apr 2019 13:36:18 -0700 Subject: [PATCH 1/9] Add full support for configuring exponential connection backoff params. --- backoff.go | 22 +++++ balancer/grpclb/grpclb.go | 8 +- balancer/xds/xds_client.go | 8 +- clientconn.go | 6 +- clientconn_state_transition_test.go | 3 +- clientconn_test.go | 37 ++++---- dialoptions.go | 22 +++-- health/client.go | 4 +- internal/backoff/backoff.go | 135 +++++++++++++++++++++++++--- internal/backoff/backoff_test.go | 54 +++++++++++ resolver/dns/dns_resolver.go | 4 +- 11 files changed, 248 insertions(+), 55 deletions(-) create mode 100644 internal/backoff/backoff_test.go diff --git a/backoff.go b/backoff.go index 97c6e2568f45..285a8523fdc6 100644 --- a/backoff.go +++ b/backoff.go @@ -27,12 +27,34 @@ import ( // DefaultBackoffConfig uses values specified for backoff in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +// Deprecated: use ConnectRetryParams instead. var DefaultBackoffConfig = BackoffConfig{ MaxDelay: 120 * time.Second, } // BackoffConfig defines the parameters for the default gRPC backoff strategy. +// Deprecated: use ConnectRetryParams instead. type BackoffConfig struct { // MaxDelay is the upper bound of backoff delay. MaxDelay time.Duration } + +// ConnectRetryParams defines the parameters for the backoff during connection +// retries. Users are encouraged to use this instead of BackoffConfig. +// +// This API is EXPERIMENTAL. +type ConnectRetryParams struct { + // BaseDelay is the amount of time to backoff after the first connection + // failure. + BaseDelay time.Duration + // Multiplier is the factor with which to multiply backoffs after a failed + // retry. + Multiplier float64 + // Jitter is the factor with which backoffs are randomized. + Jitter float64 + // MaxDelay is the upper bound of backoff delay. + MaxDelay time.Duration + // MinConnectTimeout is the minimum amount of time we are willing to give a + // connection to complete. + MinConnectTimeout time.Duration +} diff --git a/balancer/grpclb/grpclb.go b/balancer/grpclb/grpclb.go index e385d02d119b..6c806e4e6914 100644 --- a/balancer/grpclb/grpclb.go +++ b/balancer/grpclb/grpclb.go @@ -51,16 +51,14 @@ const ( ) var ( - // defaultBackoffConfig configures the backoff strategy that's used when the + // defaultBackoffStrategy configures the backoff strategy that's used when the // init handshake in the RPC is unsuccessful. It's not for the clientconn // reconnect backoff. // // It has the same value as the default grpc.DefaultBackoffConfig. // // TODO: make backoff configurable. - defaultBackoffConfig = backoff.Exponential{ - MaxDelay: 120 * time.Second, - } + defaultBackoffStrategy = backoff.NewExponentialBuilder().Build() errServerTerminatedConnection = errors.New("grpclb: failed to recv server list: server terminated connection") ) @@ -177,7 +175,7 @@ func (b *lbBuilder) Build(cc balancer.ClientConn, opt balancer.BuildOptions) bal scStates: make(map[balancer.SubConn]connectivity.State), picker: &errPicker{err: balancer.ErrNoSubConnAvailable}, clientStats: newRPCStats(), - backoff: defaultBackoffConfig, // TODO: make backoff configurable. + backoff: defaultBackoffStrategy, // TODO: make backoff configurable. } var err error diff --git a/balancer/xds/xds_client.go b/balancer/xds/xds_client.go index d18d9033abbe..7620e1d886f5 100644 --- a/balancer/xds/xds_client.go +++ b/balancer/xds/xds_client.go @@ -42,11 +42,7 @@ const ( endpointRequired = "endpoints_required" ) -var ( - defaultBackoffConfig = backoff.Exponential{ - MaxDelay: 120 * time.Second, - } -) +var defaultBackoffStrategy = backoff.NewExponentialBuilder().Build() // client is responsible for connecting to the specified traffic director, passing the received // ADS response from the traffic director, and sending notification when communication with the @@ -255,7 +251,7 @@ func newXDSClient(balancerName string, serviceName string, enableCDS bool, opts newADS: newADS, loseContact: loseContact, cleanup: exitCleanup, - backoff: defaultBackoffConfig, + backoff: defaultBackoffStrategy, } c.ctx, c.cancel = context.WithCancel(context.Background()) diff --git a/clientconn.go b/clientconn.go index a1e1a98006a7..869f6e71661b 100644 --- a/clientconn.go +++ b/clientconn.go @@ -221,9 +221,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * } } if cc.dopts.bs == nil { - cc.dopts.bs = backoff.Exponential{ - MaxDelay: DefaultBackoffConfig.MaxDelay, - } + cc.dopts.bs = backoff.NewExponentialBuilder().Build() } if cc.dopts.resolverBuilder == nil { // Only try to parse target when resolver builder is not already set. @@ -902,7 +900,7 @@ func (ac *addrConn) resetTransport() { addrs := ac.addrs backoffFor := ac.dopts.bs.Backoff(ac.backoffIdx) // This will be the duration that dial gets to finish. - dialDuration := minConnectTimeout + dialDuration := ac.dopts.bs.MinConnectionTimeout() if ac.dopts.minConnectTimeout != nil { dialDuration = ac.dopts.minConnectTimeout() } diff --git a/clientconn_state_transition_test.go b/clientconn_state_transition_test.go index 3a1513d170c6..6617044ef3a4 100644 --- a/clientconn_state_transition_test.go +++ b/clientconn_state_transition_test.go @@ -500,7 +500,8 @@ func (b *stateRecordingBalancerBuilder) nextStateNotifier() <-chan connectivity. type noBackoff struct{} -func (b noBackoff) Backoff(int) time.Duration { return time.Duration(0) } +func (b noBackoff) Backoff(int) time.Duration { return time.Duration(0) } +func (b noBackoff) MinConnectionTimeout() time.Duration { return time.Duration(0) } // Keep reading until something causes the connection to die (EOF, server // closed, etc). Useful as a tool for mindlessly keeping the connection diff --git a/clientconn_test.go b/clientconn_test.go index 9b5ac03547b6..75dcf7114d9a 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -777,22 +777,31 @@ func (s) TestCredentialsMisuse(t *testing.T) { } func (s) TestWithBackoffConfigDefault(t *testing.T) { - testBackoffConfigSet(t, &DefaultBackoffConfig) + testBackoffStrategySet(t, backoff.NewExponentialBuilder().Build()) } func (s) TestWithBackoffConfig(t *testing.T) { - b := BackoffConfig{MaxDelay: DefaultBackoffConfig.MaxDelay / 2} - expected := b - testBackoffConfigSet(t, &expected, WithBackoffConfig(b)) + md := DefaultBackoffConfig.MaxDelay / 2 + expected := backoff.NewExponentialBuilder().MaxDelay(md).Build() + testBackoffStrategySet(t, expected, WithBackoffConfig(BackoffConfig{MaxDelay: md})) } func (s) TestWithBackoffMaxDelay(t *testing.T) { md := DefaultBackoffConfig.MaxDelay / 2 - expected := BackoffConfig{MaxDelay: md} - testBackoffConfigSet(t, &expected, WithBackoffMaxDelay(md)) + expected := backoff.NewExponentialBuilder().MaxDelay(md).Build() + testBackoffStrategySet(t, expected, WithBackoffMaxDelay(md)) +} + +func (s) TestWithConnectRetryParams(t *testing.T) { + bd := 2 * time.Second + mltpr := 2.0 + jitter := 0.0 + crt := ConnectRetryParams{BaseDelay: bd, Multiplier: mltpr, Jitter: jitter} + expected := backoff.NewExponentialBuilder().BaseDelay(bd).Multiplier(mltpr).Jitter(jitter).MaxDelay(time.Duration(0)).MinConnectTimeout(time.Duration(0)).Build() + testBackoffStrategySet(t, expected, WithConnectRetryParams(crt)) } -func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOption) { +func testBackoffStrategySet(t *testing.T, expected backoff.Strategy, opts ...DialOption) { opts = append(opts, WithInsecure()) conn, err := Dial("passthrough:///foo:80", opts...) if err != nil { @@ -801,19 +810,16 @@ func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOpt defer conn.Close() if conn.dopts.bs == nil { - t.Fatalf("backoff config not set") + t.Fatalf("backoff strategy not set") } actual, ok := conn.dopts.bs.(backoff.Exponential) if !ok { - t.Fatalf("unexpected type of backoff config: %#v", conn.dopts.bs) + t.Fatalf("unexpected type of backoff strategy: %#v", conn.dopts.bs) } - expectedValue := backoff.Exponential{ - MaxDelay: expected.MaxDelay, - } - if actual != expectedValue { - t.Fatalf("unexpected backoff config on connection: %v, want %v", actual, expected) + if actual != expected.(backoff.Exponential) { + t.Errorf("unexpected backoff strategy on connection: %v, want %v", actual, expected) } } @@ -1016,7 +1022,8 @@ func (s) TestGetClientConnTarget(t *testing.T) { type backoffForever struct{} -func (b backoffForever) Backoff(int) time.Duration { return time.Duration(math.MaxInt64) } +func (b backoffForever) Backoff(int) time.Duration { return time.Duration(math.MaxInt64) } +func (b backoffForever) MinConnectionTimeout() time.Duration { return time.Duration(0) } func (s) TestResetConnectBackoff(t *testing.T) { dials := make(chan struct{}) diff --git a/dialoptions.go b/dialoptions.go index a0743a9e75fa..441741a2ab1e 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -247,8 +247,23 @@ func WithServiceConfig(c <-chan ServiceConfig) DialOption { }) } +// WithConnectRetryParams configures the dialer to use the provided backoff +// parameters for the algorithm defined in +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +// This will override all the default values with the ones provided here. So, +// use with caution. +// +// This API is EXPERIMENTAL. +func WithConnectRetryParams(p ConnectRetryParams) DialOption { + b := backoff.NewExponentialBuilder() + b.BaseDelay(p.BaseDelay).Multiplier(p.Multiplier).Jitter(p.Jitter).MaxDelay(p.MaxDelay).MinConnectTimeout(p.MinConnectTimeout) + return withBackoff(b.Build()) +} + // WithBackoffMaxDelay configures the dialer to use the provided maximum delay // when backing off after failed connection attempts. +// +// Deprecated: use WithConnectRetryParams instead. func WithBackoffMaxDelay(md time.Duration) DialOption { return WithBackoffConfig(BackoffConfig{MaxDelay: md}) } @@ -256,12 +271,9 @@ func WithBackoffMaxDelay(md time.Duration) DialOption { // WithBackoffConfig configures the dialer to use the provided backoff // parameters after connection failures. // -// Use WithBackoffMaxDelay until more parameters on BackoffConfig are opened up -// for use. +// Deprecated: use WithConnectRetryParams instead. func WithBackoffConfig(b BackoffConfig) DialOption { - return withBackoff(backoff.Exponential{ - MaxDelay: b.MaxDelay, - }) + return withBackoff(backoff.NewExponentialBuilder().MaxDelay(b.MaxDelay).Build()) } // withBackoff sets the backoff strategy used for connectRetryNum after a failed diff --git a/health/client.go b/health/client.go index e15f04c229c4..73c5a6690611 100644 --- a/health/client.go +++ b/health/client.go @@ -32,9 +32,7 @@ import ( "google.golang.org/grpc/status" ) -const maxDelay = 120 * time.Second - -var backoffStrategy = backoff.Exponential{MaxDelay: maxDelay} +var backoffStrategy = backoff.NewExponentialBuilder().Build() var backoffFunc = func(ctx context.Context, retries int) bool { d := backoffStrategy.Backoff(retries) timer := time.NewTimer(d) diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go index 1bd0cce5abdf..712362d22c0f 100644 --- a/internal/backoff/backoff.go +++ b/internal/backoff/backoff.go @@ -30,39 +30,139 @@ import ( // Strategy defines the methodology for backing off after a grpc connection // failure. -// type Strategy interface { // Backoff returns the amount of time to wait before the next retry given // the number of consecutive failures. Backoff(retries int) time.Duration + // MinConnectionTimeout returns the minimum amount of time we are willing to + // give a connection to complete. + MinConnectionTimeout() time.Duration } -const ( - // baseDelay is the amount of time to wait before retrying after the first +// StrategyBuilder helps build an implementation of the Strategy interface, +// with setters provided to set individual parameters of the backoff strategy. +type StrategyBuilder interface { + // BaseDelay sets the amount of time to backoff after the first connection // failure. - baseDelay = 1.0 * time.Second - // factor is applied to the backoff after each retry. - factor = 1.6 - // jitter provides a range to randomize backoff delays. - jitter = 0.2 + BaseDelay(time.Duration) StrategyBuilder + // Multiplier sets the the factor with which to multiply backoffs after a + // failed retry. + Multiplier(float64) StrategyBuilder + // Jitter sets the factor with which backoffs are randomized. + Jitter(float64) StrategyBuilder + // MaxDelay sets the upper bound of backoff delay. + MaxDelay(time.Duration) StrategyBuilder + // MinConnectTimeout sets the minimum amount of time we are willing to give a + // connection to complete. + MinConnectTimeout(time.Duration) StrategyBuilder + // Build builds an implementation of the Strategy interface configured using + // the above setter methods. + Build() Strategy +} + +// Default values for the different backoff parameters as defined in +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +const ( + baseDelay = 1.0 * time.Second + multiplier = 1.6 + jitter = 0.2 + maxDelay = 120 * time.Second + minConnectTimeout = 20 * time.Second ) +// NewExponentialBuilder returns an implementation of the StrategyBuilder +// interface used to build an exponential backoff strategy implementation, as +// defined in +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +func NewExponentialBuilder() StrategyBuilder { + return &exponentialBuilder{ + bd: baseDelay, + mltpr: multiplier, + jitter: jitter, + md: maxDelay, + mct: minConnectTimeout, + } +} + +// exponentialBuilder implements the StrategyBuilder interface for an +// exponential backoff strategy. +type exponentialBuilder struct { + bd time.Duration + mltpr float64 + jitter float64 + md time.Duration + mct time.Duration +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) BaseDelay(bd time.Duration) StrategyBuilder { + eb.bd = bd + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) Multiplier(m float64) StrategyBuilder { + eb.mltpr = m + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) Jitter(j float64) StrategyBuilder { + eb.jitter = j + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) MaxDelay(md time.Duration) StrategyBuilder { + eb.md = md + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) MinConnectTimeout(mct time.Duration) StrategyBuilder { + eb.mct = mct + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) Build() Strategy { + return Exponential{ + baseDelay: eb.bd, + multiplier: eb.mltpr, + jitter: eb.jitter, + maxDelay: eb.md, + minConnectTimeout: eb.mct, + } +} + // Exponential implements exponential backoff algorithm as defined in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. type Exponential struct { - // MaxDelay is the upper bound of backoff delay. - MaxDelay time.Duration + // baseDelay is the amount of time to backoff after the first connection + // failure. + baseDelay time.Duration + // multiplier is the factor with which to multiply backoffs after a failed + // retry. + multiplier float64 + // jitter is the factor with which backoffs are randomized. + jitter float64 + // maxDelay is the upper bound of backoff delay. + maxDelay time.Duration + // minConnectTimeout is the minimum amount of time we are willing to give a + // connection to complete. + minConnectTimeout time.Duration } // Backoff returns the amount of time to wait before the next retry given the // number of retries. +// Helps implement Strategy. func (bc Exponential) Backoff(retries int) time.Duration { if retries == 0 { - return baseDelay + return bc.baseDelay } - backoff, max := float64(baseDelay), float64(bc.MaxDelay) + backoff, max := float64(bc.baseDelay), float64(bc.maxDelay) for backoff < max && retries > 0 { - backoff *= factor + backoff *= bc.multiplier retries-- } if backoff > max { @@ -70,9 +170,16 @@ func (bc Exponential) Backoff(retries int) time.Duration { } // Randomize backoff delays so that if a cluster of requests start at // the same time, they won't operate in lockstep. - backoff *= 1 + jitter*(grpcrand.Float64()*2-1) + backoff *= 1 + bc.jitter*(grpcrand.Float64()*2-1) if backoff < 0 { return 0 } return time.Duration(backoff) } + +// MinConnectionTimeout returns the minimum amount of time we are willing to +// give a connection to complete. +// Helps implement Strategy. +func (bc Exponential) MinConnectionTimeout() time.Duration { + return bc.minConnectTimeout +} diff --git a/internal/backoff/backoff_test.go b/internal/backoff/backoff_test.go new file mode 100644 index 000000000000..78ff1ed53d0f --- /dev/null +++ b/internal/backoff/backoff_test.go @@ -0,0 +1,54 @@ +/* + * + * Copyright 2019 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package backoff + +import ( + "testing" + "time" +) + +func TestNewExponentialBuilderDefault(t *testing.T) { + gotExp := NewExponentialBuilder().Build().(Strategy) + wantExp := Exponential{ + baseDelay: baseDelay, + multiplier: multiplier, + jitter: jitter, + maxDelay: maxDelay, + minConnectTimeout: minConnectTimeout, + } + if gotExp != wantExp { + t.Errorf("DefaultExponentialBuilder got: %v, want %v", gotExp, wantExp) + } +} + +func TestNewExponentialBuilderOverride(t *testing.T) { + bd := 2 * time.Second + mltpr := 2.0 + gotExp := NewExponentialBuilder().BaseDelay(bd).Multiplier(mltpr).Build().(Strategy) + wantExp := Exponential{ + baseDelay: bd, + multiplier: mltpr, + jitter: jitter, + maxDelay: maxDelay, + minConnectTimeout: minConnectTimeout, + } + if gotExp != wantExp { + t.Errorf("OverrideExponentialBuilder got: %v, want %v", gotExp, wantExp) + } +} diff --git a/resolver/dns/dns_resolver.go b/resolver/dns/dns_resolver.go index 58355990779b..3122eddb0f22 100644 --- a/resolver/dns/dns_resolver.go +++ b/resolver/dns/dns_resolver.go @@ -125,7 +125,7 @@ func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts ctx, cancel := context.WithCancel(context.Background()) d := &dnsResolver{ freq: b.minFreq, - backoff: backoff.Exponential{MaxDelay: b.minFreq}, + backoff: backoff.NewExponentialBuilder().MaxDelay(b.minFreq).Build(), host: host, port: port, ctx: ctx, @@ -197,7 +197,7 @@ func (i *ipResolver) watcher() { // dnsResolver watches for the name resolution update for a non-IP target. type dnsResolver struct { freq time.Duration - backoff backoff.Exponential + backoff backoff.Strategy retryCount int host string port string From dffc6d612692e873e4027f553c6b50da18e3faac Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Tue, 2 Apr 2019 13:36:18 -0700 Subject: [PATCH 2/9] Add full support for configuring exponential connection backoff params. --- backoff.go | 22 +++++ balancer/grpclb/grpclb.go | 8 +- balancer/xds/xds_client.go | 8 +- clientconn.go | 8 +- clientconn_state_transition_test.go | 3 +- clientconn_test.go | 37 ++++---- dialoptions.go | 22 +++-- health/client.go | 4 +- internal/backoff/backoff.go | 135 +++++++++++++++++++++++++--- internal/backoff/backoff_test.go | 54 +++++++++++ resolver/dns/dns_resolver.go | 4 +- 11 files changed, 248 insertions(+), 57 deletions(-) create mode 100644 internal/backoff/backoff_test.go diff --git a/backoff.go b/backoff.go index 97c6e2568f45..285a8523fdc6 100644 --- a/backoff.go +++ b/backoff.go @@ -27,12 +27,34 @@ import ( // DefaultBackoffConfig uses values specified for backoff in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +// Deprecated: use ConnectRetryParams instead. var DefaultBackoffConfig = BackoffConfig{ MaxDelay: 120 * time.Second, } // BackoffConfig defines the parameters for the default gRPC backoff strategy. +// Deprecated: use ConnectRetryParams instead. type BackoffConfig struct { // MaxDelay is the upper bound of backoff delay. MaxDelay time.Duration } + +// ConnectRetryParams defines the parameters for the backoff during connection +// retries. Users are encouraged to use this instead of BackoffConfig. +// +// This API is EXPERIMENTAL. +type ConnectRetryParams struct { + // BaseDelay is the amount of time to backoff after the first connection + // failure. + BaseDelay time.Duration + // Multiplier is the factor with which to multiply backoffs after a failed + // retry. + Multiplier float64 + // Jitter is the factor with which backoffs are randomized. + Jitter float64 + // MaxDelay is the upper bound of backoff delay. + MaxDelay time.Duration + // MinConnectTimeout is the minimum amount of time we are willing to give a + // connection to complete. + MinConnectTimeout time.Duration +} diff --git a/balancer/grpclb/grpclb.go b/balancer/grpclb/grpclb.go index e385d02d119b..6c806e4e6914 100644 --- a/balancer/grpclb/grpclb.go +++ b/balancer/grpclb/grpclb.go @@ -51,16 +51,14 @@ const ( ) var ( - // defaultBackoffConfig configures the backoff strategy that's used when the + // defaultBackoffStrategy configures the backoff strategy that's used when the // init handshake in the RPC is unsuccessful. It's not for the clientconn // reconnect backoff. // // It has the same value as the default grpc.DefaultBackoffConfig. // // TODO: make backoff configurable. - defaultBackoffConfig = backoff.Exponential{ - MaxDelay: 120 * time.Second, - } + defaultBackoffStrategy = backoff.NewExponentialBuilder().Build() errServerTerminatedConnection = errors.New("grpclb: failed to recv server list: server terminated connection") ) @@ -177,7 +175,7 @@ func (b *lbBuilder) Build(cc balancer.ClientConn, opt balancer.BuildOptions) bal scStates: make(map[balancer.SubConn]connectivity.State), picker: &errPicker{err: balancer.ErrNoSubConnAvailable}, clientStats: newRPCStats(), - backoff: defaultBackoffConfig, // TODO: make backoff configurable. + backoff: defaultBackoffStrategy, // TODO: make backoff configurable. } var err error diff --git a/balancer/xds/xds_client.go b/balancer/xds/xds_client.go index d18d9033abbe..7620e1d886f5 100644 --- a/balancer/xds/xds_client.go +++ b/balancer/xds/xds_client.go @@ -42,11 +42,7 @@ const ( endpointRequired = "endpoints_required" ) -var ( - defaultBackoffConfig = backoff.Exponential{ - MaxDelay: 120 * time.Second, - } -) +var defaultBackoffStrategy = backoff.NewExponentialBuilder().Build() // client is responsible for connecting to the specified traffic director, passing the received // ADS response from the traffic director, and sending notification when communication with the @@ -255,7 +251,7 @@ func newXDSClient(balancerName string, serviceName string, enableCDS bool, opts newADS: newADS, loseContact: loseContact, cleanup: exitCleanup, - backoff: defaultBackoffConfig, + backoff: defaultBackoffStrategy, } c.ctx, c.cancel = context.WithCancel(context.Background()) diff --git a/clientconn.go b/clientconn.go index a1e1a98006a7..eed61d57528d 100644 --- a/clientconn.go +++ b/clientconn.go @@ -49,8 +49,6 @@ import ( ) const ( - // minimum time to give a connection to complete - minConnectTimeout = 20 * time.Second // must match grpclbName in grpclb/grpclb.go grpclbName = "grpclb" ) @@ -221,9 +219,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * } } if cc.dopts.bs == nil { - cc.dopts.bs = backoff.Exponential{ - MaxDelay: DefaultBackoffConfig.MaxDelay, - } + cc.dopts.bs = backoff.NewExponentialBuilder().Build() } if cc.dopts.resolverBuilder == nil { // Only try to parse target when resolver builder is not already set. @@ -902,7 +898,7 @@ func (ac *addrConn) resetTransport() { addrs := ac.addrs backoffFor := ac.dopts.bs.Backoff(ac.backoffIdx) // This will be the duration that dial gets to finish. - dialDuration := minConnectTimeout + dialDuration := ac.dopts.bs.MinConnectionTimeout() if ac.dopts.minConnectTimeout != nil { dialDuration = ac.dopts.minConnectTimeout() } diff --git a/clientconn_state_transition_test.go b/clientconn_state_transition_test.go index 3a1513d170c6..6617044ef3a4 100644 --- a/clientconn_state_transition_test.go +++ b/clientconn_state_transition_test.go @@ -500,7 +500,8 @@ func (b *stateRecordingBalancerBuilder) nextStateNotifier() <-chan connectivity. type noBackoff struct{} -func (b noBackoff) Backoff(int) time.Duration { return time.Duration(0) } +func (b noBackoff) Backoff(int) time.Duration { return time.Duration(0) } +func (b noBackoff) MinConnectionTimeout() time.Duration { return time.Duration(0) } // Keep reading until something causes the connection to die (EOF, server // closed, etc). Useful as a tool for mindlessly keeping the connection diff --git a/clientconn_test.go b/clientconn_test.go index 9b5ac03547b6..75dcf7114d9a 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -777,22 +777,31 @@ func (s) TestCredentialsMisuse(t *testing.T) { } func (s) TestWithBackoffConfigDefault(t *testing.T) { - testBackoffConfigSet(t, &DefaultBackoffConfig) + testBackoffStrategySet(t, backoff.NewExponentialBuilder().Build()) } func (s) TestWithBackoffConfig(t *testing.T) { - b := BackoffConfig{MaxDelay: DefaultBackoffConfig.MaxDelay / 2} - expected := b - testBackoffConfigSet(t, &expected, WithBackoffConfig(b)) + md := DefaultBackoffConfig.MaxDelay / 2 + expected := backoff.NewExponentialBuilder().MaxDelay(md).Build() + testBackoffStrategySet(t, expected, WithBackoffConfig(BackoffConfig{MaxDelay: md})) } func (s) TestWithBackoffMaxDelay(t *testing.T) { md := DefaultBackoffConfig.MaxDelay / 2 - expected := BackoffConfig{MaxDelay: md} - testBackoffConfigSet(t, &expected, WithBackoffMaxDelay(md)) + expected := backoff.NewExponentialBuilder().MaxDelay(md).Build() + testBackoffStrategySet(t, expected, WithBackoffMaxDelay(md)) +} + +func (s) TestWithConnectRetryParams(t *testing.T) { + bd := 2 * time.Second + mltpr := 2.0 + jitter := 0.0 + crt := ConnectRetryParams{BaseDelay: bd, Multiplier: mltpr, Jitter: jitter} + expected := backoff.NewExponentialBuilder().BaseDelay(bd).Multiplier(mltpr).Jitter(jitter).MaxDelay(time.Duration(0)).MinConnectTimeout(time.Duration(0)).Build() + testBackoffStrategySet(t, expected, WithConnectRetryParams(crt)) } -func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOption) { +func testBackoffStrategySet(t *testing.T, expected backoff.Strategy, opts ...DialOption) { opts = append(opts, WithInsecure()) conn, err := Dial("passthrough:///foo:80", opts...) if err != nil { @@ -801,19 +810,16 @@ func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOpt defer conn.Close() if conn.dopts.bs == nil { - t.Fatalf("backoff config not set") + t.Fatalf("backoff strategy not set") } actual, ok := conn.dopts.bs.(backoff.Exponential) if !ok { - t.Fatalf("unexpected type of backoff config: %#v", conn.dopts.bs) + t.Fatalf("unexpected type of backoff strategy: %#v", conn.dopts.bs) } - expectedValue := backoff.Exponential{ - MaxDelay: expected.MaxDelay, - } - if actual != expectedValue { - t.Fatalf("unexpected backoff config on connection: %v, want %v", actual, expected) + if actual != expected.(backoff.Exponential) { + t.Errorf("unexpected backoff strategy on connection: %v, want %v", actual, expected) } } @@ -1016,7 +1022,8 @@ func (s) TestGetClientConnTarget(t *testing.T) { type backoffForever struct{} -func (b backoffForever) Backoff(int) time.Duration { return time.Duration(math.MaxInt64) } +func (b backoffForever) Backoff(int) time.Duration { return time.Duration(math.MaxInt64) } +func (b backoffForever) MinConnectionTimeout() time.Duration { return time.Duration(0) } func (s) TestResetConnectBackoff(t *testing.T) { dials := make(chan struct{}) diff --git a/dialoptions.go b/dialoptions.go index a0743a9e75fa..441741a2ab1e 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -247,8 +247,23 @@ func WithServiceConfig(c <-chan ServiceConfig) DialOption { }) } +// WithConnectRetryParams configures the dialer to use the provided backoff +// parameters for the algorithm defined in +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +// This will override all the default values with the ones provided here. So, +// use with caution. +// +// This API is EXPERIMENTAL. +func WithConnectRetryParams(p ConnectRetryParams) DialOption { + b := backoff.NewExponentialBuilder() + b.BaseDelay(p.BaseDelay).Multiplier(p.Multiplier).Jitter(p.Jitter).MaxDelay(p.MaxDelay).MinConnectTimeout(p.MinConnectTimeout) + return withBackoff(b.Build()) +} + // WithBackoffMaxDelay configures the dialer to use the provided maximum delay // when backing off after failed connection attempts. +// +// Deprecated: use WithConnectRetryParams instead. func WithBackoffMaxDelay(md time.Duration) DialOption { return WithBackoffConfig(BackoffConfig{MaxDelay: md}) } @@ -256,12 +271,9 @@ func WithBackoffMaxDelay(md time.Duration) DialOption { // WithBackoffConfig configures the dialer to use the provided backoff // parameters after connection failures. // -// Use WithBackoffMaxDelay until more parameters on BackoffConfig are opened up -// for use. +// Deprecated: use WithConnectRetryParams instead. func WithBackoffConfig(b BackoffConfig) DialOption { - return withBackoff(backoff.Exponential{ - MaxDelay: b.MaxDelay, - }) + return withBackoff(backoff.NewExponentialBuilder().MaxDelay(b.MaxDelay).Build()) } // withBackoff sets the backoff strategy used for connectRetryNum after a failed diff --git a/health/client.go b/health/client.go index e15f04c229c4..73c5a6690611 100644 --- a/health/client.go +++ b/health/client.go @@ -32,9 +32,7 @@ import ( "google.golang.org/grpc/status" ) -const maxDelay = 120 * time.Second - -var backoffStrategy = backoff.Exponential{MaxDelay: maxDelay} +var backoffStrategy = backoff.NewExponentialBuilder().Build() var backoffFunc = func(ctx context.Context, retries int) bool { d := backoffStrategy.Backoff(retries) timer := time.NewTimer(d) diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go index 1bd0cce5abdf..712362d22c0f 100644 --- a/internal/backoff/backoff.go +++ b/internal/backoff/backoff.go @@ -30,39 +30,139 @@ import ( // Strategy defines the methodology for backing off after a grpc connection // failure. -// type Strategy interface { // Backoff returns the amount of time to wait before the next retry given // the number of consecutive failures. Backoff(retries int) time.Duration + // MinConnectionTimeout returns the minimum amount of time we are willing to + // give a connection to complete. + MinConnectionTimeout() time.Duration } -const ( - // baseDelay is the amount of time to wait before retrying after the first +// StrategyBuilder helps build an implementation of the Strategy interface, +// with setters provided to set individual parameters of the backoff strategy. +type StrategyBuilder interface { + // BaseDelay sets the amount of time to backoff after the first connection // failure. - baseDelay = 1.0 * time.Second - // factor is applied to the backoff after each retry. - factor = 1.6 - // jitter provides a range to randomize backoff delays. - jitter = 0.2 + BaseDelay(time.Duration) StrategyBuilder + // Multiplier sets the the factor with which to multiply backoffs after a + // failed retry. + Multiplier(float64) StrategyBuilder + // Jitter sets the factor with which backoffs are randomized. + Jitter(float64) StrategyBuilder + // MaxDelay sets the upper bound of backoff delay. + MaxDelay(time.Duration) StrategyBuilder + // MinConnectTimeout sets the minimum amount of time we are willing to give a + // connection to complete. + MinConnectTimeout(time.Duration) StrategyBuilder + // Build builds an implementation of the Strategy interface configured using + // the above setter methods. + Build() Strategy +} + +// Default values for the different backoff parameters as defined in +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +const ( + baseDelay = 1.0 * time.Second + multiplier = 1.6 + jitter = 0.2 + maxDelay = 120 * time.Second + minConnectTimeout = 20 * time.Second ) +// NewExponentialBuilder returns an implementation of the StrategyBuilder +// interface used to build an exponential backoff strategy implementation, as +// defined in +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +func NewExponentialBuilder() StrategyBuilder { + return &exponentialBuilder{ + bd: baseDelay, + mltpr: multiplier, + jitter: jitter, + md: maxDelay, + mct: minConnectTimeout, + } +} + +// exponentialBuilder implements the StrategyBuilder interface for an +// exponential backoff strategy. +type exponentialBuilder struct { + bd time.Duration + mltpr float64 + jitter float64 + md time.Duration + mct time.Duration +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) BaseDelay(bd time.Duration) StrategyBuilder { + eb.bd = bd + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) Multiplier(m float64) StrategyBuilder { + eb.mltpr = m + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) Jitter(j float64) StrategyBuilder { + eb.jitter = j + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) MaxDelay(md time.Duration) StrategyBuilder { + eb.md = md + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) MinConnectTimeout(mct time.Duration) StrategyBuilder { + eb.mct = mct + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) Build() Strategy { + return Exponential{ + baseDelay: eb.bd, + multiplier: eb.mltpr, + jitter: eb.jitter, + maxDelay: eb.md, + minConnectTimeout: eb.mct, + } +} + // Exponential implements exponential backoff algorithm as defined in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. type Exponential struct { - // MaxDelay is the upper bound of backoff delay. - MaxDelay time.Duration + // baseDelay is the amount of time to backoff after the first connection + // failure. + baseDelay time.Duration + // multiplier is the factor with which to multiply backoffs after a failed + // retry. + multiplier float64 + // jitter is the factor with which backoffs are randomized. + jitter float64 + // maxDelay is the upper bound of backoff delay. + maxDelay time.Duration + // minConnectTimeout is the minimum amount of time we are willing to give a + // connection to complete. + minConnectTimeout time.Duration } // Backoff returns the amount of time to wait before the next retry given the // number of retries. +// Helps implement Strategy. func (bc Exponential) Backoff(retries int) time.Duration { if retries == 0 { - return baseDelay + return bc.baseDelay } - backoff, max := float64(baseDelay), float64(bc.MaxDelay) + backoff, max := float64(bc.baseDelay), float64(bc.maxDelay) for backoff < max && retries > 0 { - backoff *= factor + backoff *= bc.multiplier retries-- } if backoff > max { @@ -70,9 +170,16 @@ func (bc Exponential) Backoff(retries int) time.Duration { } // Randomize backoff delays so that if a cluster of requests start at // the same time, they won't operate in lockstep. - backoff *= 1 + jitter*(grpcrand.Float64()*2-1) + backoff *= 1 + bc.jitter*(grpcrand.Float64()*2-1) if backoff < 0 { return 0 } return time.Duration(backoff) } + +// MinConnectionTimeout returns the minimum amount of time we are willing to +// give a connection to complete. +// Helps implement Strategy. +func (bc Exponential) MinConnectionTimeout() time.Duration { + return bc.minConnectTimeout +} diff --git a/internal/backoff/backoff_test.go b/internal/backoff/backoff_test.go new file mode 100644 index 000000000000..78ff1ed53d0f --- /dev/null +++ b/internal/backoff/backoff_test.go @@ -0,0 +1,54 @@ +/* + * + * Copyright 2019 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package backoff + +import ( + "testing" + "time" +) + +func TestNewExponentialBuilderDefault(t *testing.T) { + gotExp := NewExponentialBuilder().Build().(Strategy) + wantExp := Exponential{ + baseDelay: baseDelay, + multiplier: multiplier, + jitter: jitter, + maxDelay: maxDelay, + minConnectTimeout: minConnectTimeout, + } + if gotExp != wantExp { + t.Errorf("DefaultExponentialBuilder got: %v, want %v", gotExp, wantExp) + } +} + +func TestNewExponentialBuilderOverride(t *testing.T) { + bd := 2 * time.Second + mltpr := 2.0 + gotExp := NewExponentialBuilder().BaseDelay(bd).Multiplier(mltpr).Build().(Strategy) + wantExp := Exponential{ + baseDelay: bd, + multiplier: mltpr, + jitter: jitter, + maxDelay: maxDelay, + minConnectTimeout: minConnectTimeout, + } + if gotExp != wantExp { + t.Errorf("OverrideExponentialBuilder got: %v, want %v", gotExp, wantExp) + } +} diff --git a/resolver/dns/dns_resolver.go b/resolver/dns/dns_resolver.go index 58355990779b..3122eddb0f22 100644 --- a/resolver/dns/dns_resolver.go +++ b/resolver/dns/dns_resolver.go @@ -125,7 +125,7 @@ func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts ctx, cancel := context.WithCancel(context.Background()) d := &dnsResolver{ freq: b.minFreq, - backoff: backoff.Exponential{MaxDelay: b.minFreq}, + backoff: backoff.NewExponentialBuilder().MaxDelay(b.minFreq).Build(), host: host, port: port, ctx: ctx, @@ -197,7 +197,7 @@ func (i *ipResolver) watcher() { // dnsResolver watches for the name resolution update for a non-IP target. type dnsResolver struct { freq time.Duration - backoff backoff.Exponential + backoff backoff.Strategy retryCount int host string port string From 65e8212147a8f9a6e7abe6df249fb689d42ff1e9 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Thu, 11 Apr 2019 17:01:14 -0700 Subject: [PATCH 3/9] Handle review comments. Move MinConnectionTimeout() out of backoff.Strategy interface. Store it directly in dialOptions instead, and have ClientConn use it from there. --- backoff.go | 31 +++++++++++----------- clientconn.go | 2 +- clientconn_state_transition_test.go | 3 +-- clientconn_test.go | 11 ++++---- dialoptions.go | 22 +++++++++++----- internal/backoff/backoff.go | 41 ++++++----------------------- internal/backoff/backoff_test.go | 18 ++++++------- 7 files changed, 55 insertions(+), 73 deletions(-) diff --git a/backoff.go b/backoff.go index 285a8523fdc6..d9de4b9f83fe 100644 --- a/backoff.go +++ b/backoff.go @@ -27,33 +27,34 @@ import ( // DefaultBackoffConfig uses values specified for backoff in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. -// Deprecated: use ConnectRetryParams instead. +// Deprecated: use ConnectParams instead. var DefaultBackoffConfig = BackoffConfig{ MaxDelay: 120 * time.Second, } // BackoffConfig defines the parameters for the default gRPC backoff strategy. -// Deprecated: use ConnectRetryParams instead. +// Deprecated: use ConnectParams instead. type BackoffConfig struct { // MaxDelay is the upper bound of backoff delay. MaxDelay time.Duration } -// ConnectRetryParams defines the parameters for the backoff during connection -// retries. Users are encouraged to use this instead of BackoffConfig. +// ConnectParams defines the parameters for connecting and retrying. Users +// are encouraged to use this instead of BackoffConfig. +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. // // This API is EXPERIMENTAL. -type ConnectRetryParams struct { - // BaseDelay is the amount of time to backoff after the first connection - // failure. - BaseDelay time.Duration - // Multiplier is the factor with which to multiply backoffs after a failed - // retry. - Multiplier float64 - // Jitter is the factor with which backoffs are randomized. - Jitter float64 - // MaxDelay is the upper bound of backoff delay. - MaxDelay time.Duration +type ConnectParams struct { + // BackoffBaseDelay is the amount of time to backoff after the first + // connection failure. + BackoffBaseDelay time.Duration + // BackoffMultiplier is the factor with which to multiply backoffs after a + // failed retry. Should ideally be greater than 1. + BackoffMultiplier float64 + // BackoffJitter is the factor with which backoffs are randomized. + BackoffJitter float64 + // BackoffMaxDelay is the upper bound of backoff delay. + BackoffMaxDelay time.Duration // MinConnectTimeout is the minimum amount of time we are willing to give a // connection to complete. MinConnectTimeout time.Duration diff --git a/clientconn.go b/clientconn.go index 61f78a439ab0..1739bf83d001 100644 --- a/clientconn.go +++ b/clientconn.go @@ -950,7 +950,7 @@ func (ac *addrConn) resetTransport() { addrs := ac.addrs backoffFor := ac.dopts.bs.Backoff(ac.backoffIdx) // This will be the duration that dial gets to finish. - dialDuration := ac.dopts.bs.MinConnectionTimeout() + dialDuration := ac.dopts.mct if ac.dopts.minConnectTimeout != nil { dialDuration = ac.dopts.minConnectTimeout() } diff --git a/clientconn_state_transition_test.go b/clientconn_state_transition_test.go index 6617044ef3a4..3a1513d170c6 100644 --- a/clientconn_state_transition_test.go +++ b/clientconn_state_transition_test.go @@ -500,8 +500,7 @@ func (b *stateRecordingBalancerBuilder) nextStateNotifier() <-chan connectivity. type noBackoff struct{} -func (b noBackoff) Backoff(int) time.Duration { return time.Duration(0) } -func (b noBackoff) MinConnectionTimeout() time.Duration { return time.Duration(0) } +func (b noBackoff) Backoff(int) time.Duration { return time.Duration(0) } // Keep reading until something causes the connection to die (EOF, server // closed, etc). Useful as a tool for mindlessly keeping the connection diff --git a/clientconn_test.go b/clientconn_test.go index 21630d0c204d..464a0f3849ed 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -835,13 +835,13 @@ func (s) TestWithBackoffMaxDelay(t *testing.T) { testBackoffStrategySet(t, expected, WithBackoffMaxDelay(md)) } -func (s) TestWithConnectRetryParams(t *testing.T) { +func (s) TestWithConnectParams(t *testing.T) { bd := 2 * time.Second mltpr := 2.0 jitter := 0.0 - crt := ConnectRetryParams{BaseDelay: bd, Multiplier: mltpr, Jitter: jitter} - expected := backoff.NewExponentialBuilder().BaseDelay(bd).Multiplier(mltpr).Jitter(jitter).MaxDelay(time.Duration(0)).MinConnectTimeout(time.Duration(0)).Build() - testBackoffStrategySet(t, expected, WithConnectRetryParams(crt)) + crt := ConnectParams{BackoffBaseDelay: bd, BackoffMultiplier: mltpr, BackoffJitter: jitter} + expected := backoff.NewExponentialBuilder().BaseDelay(bd).Multiplier(mltpr).Jitter(jitter).MaxDelay(time.Duration(0)).Build() + testBackoffStrategySet(t, expected, WithConnectParams(crt)) } func testBackoffStrategySet(t *testing.T, expected backoff.Strategy, opts ...DialOption) { @@ -1065,8 +1065,7 @@ func (s) TestGetClientConnTarget(t *testing.T) { type backoffForever struct{} -func (b backoffForever) Backoff(int) time.Duration { return time.Duration(math.MaxInt64) } -func (b backoffForever) MinConnectionTimeout() time.Duration { return time.Duration(0) } +func (b backoffForever) Backoff(int) time.Duration { return time.Duration(math.MaxInt64) } func (s) TestResetConnectBackoff(t *testing.T) { dials := make(chan struct{}) diff --git a/dialoptions.go b/dialoptions.go index 859bd6ee8f01..ee108f2c7b81 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -36,6 +36,11 @@ import ( "google.golang.org/grpc/stats" ) +const ( + // Minimum time to give a connection to complete. + minConnectTimeout = 20 * time.Second +) + // dialOptions configure a Dial call. dialOptions are set by the DialOption // values passed to Dial. type dialOptions struct { @@ -65,6 +70,7 @@ type dialOptions struct { minConnectTimeout func() time.Duration defaultServiceConfig *ServiceConfig // defaultServiceConfig is parsed from defaultServiceConfigRawJSON. defaultServiceConfigRawJSON *string + mct time.Duration } // DialOption configures how we set up the connection. @@ -249,23 +255,26 @@ func WithServiceConfig(c <-chan ServiceConfig) DialOption { }) } -// WithConnectRetryParams configures the dialer to use the provided backoff +// WithConnectParams configures the dialer to use the provided backoff // parameters for the algorithm defined in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. // This will override all the default values with the ones provided here. So, // use with caution. // // This API is EXPERIMENTAL. -func WithConnectRetryParams(p ConnectRetryParams) DialOption { +func WithConnectParams(p ConnectParams) DialOption { b := backoff.NewExponentialBuilder() - b.BaseDelay(p.BaseDelay).Multiplier(p.Multiplier).Jitter(p.Jitter).MaxDelay(p.MaxDelay).MinConnectTimeout(p.MinConnectTimeout) - return withBackoff(b.Build()) + b.BaseDelay(p.BackoffBaseDelay).Multiplier(p.BackoffMultiplier).Jitter(p.BackoffJitter).MaxDelay(p.BackoffMaxDelay) + return newFuncDialOption(func(o *dialOptions) { + o.bs = b.Build() + o.mct = p.MinConnectTimeout + }) } // WithBackoffMaxDelay configures the dialer to use the provided maximum delay // when backing off after failed connection attempts. // -// Deprecated: use WithConnectRetryParams instead. +// Deprecated: use WithConnectParams instead. func WithBackoffMaxDelay(md time.Duration) DialOption { return WithBackoffConfig(BackoffConfig{MaxDelay: md}) } @@ -273,7 +282,7 @@ func WithBackoffMaxDelay(md time.Duration) DialOption { // WithBackoffConfig configures the dialer to use the provided backoff // parameters after connection failures. // -// Deprecated: use WithConnectRetryParams instead. +// Deprecated: use WithConnectParams instead. func WithBackoffConfig(b BackoffConfig) DialOption { return withBackoff(backoff.NewExponentialBuilder().MaxDelay(b.MaxDelay).Build()) } @@ -529,6 +538,7 @@ func defaultDialOptions() dialOptions { WriteBufferSize: defaultWriteBufSize, ReadBufferSize: defaultReadBufSize, }, + mct: minConnectTimeout, } } diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go index 712362d22c0f..4d15ce7d7c99 100644 --- a/internal/backoff/backoff.go +++ b/internal/backoff/backoff.go @@ -34,9 +34,6 @@ type Strategy interface { // Backoff returns the amount of time to wait before the next retry given // the number of consecutive failures. Backoff(retries int) time.Duration - // MinConnectionTimeout returns the minimum amount of time we are willing to - // give a connection to complete. - MinConnectionTimeout() time.Duration } // StrategyBuilder helps build an implementation of the Strategy interface, @@ -52,9 +49,6 @@ type StrategyBuilder interface { Jitter(float64) StrategyBuilder // MaxDelay sets the upper bound of backoff delay. MaxDelay(time.Duration) StrategyBuilder - // MinConnectTimeout sets the minimum amount of time we are willing to give a - // connection to complete. - MinConnectTimeout(time.Duration) StrategyBuilder // Build builds an implementation of the Strategy interface configured using // the above setter methods. Build() Strategy @@ -63,11 +57,10 @@ type StrategyBuilder interface { // Default values for the different backoff parameters as defined in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. const ( - baseDelay = 1.0 * time.Second - multiplier = 1.6 - jitter = 0.2 - maxDelay = 120 * time.Second - minConnectTimeout = 20 * time.Second + baseDelay = 1.0 * time.Second + multiplier = 1.6 + jitter = 0.2 + maxDelay = 120 * time.Second ) // NewExponentialBuilder returns an implementation of the StrategyBuilder @@ -80,7 +73,6 @@ func NewExponentialBuilder() StrategyBuilder { mltpr: multiplier, jitter: jitter, md: maxDelay, - mct: minConnectTimeout, } } @@ -118,20 +110,13 @@ func (eb *exponentialBuilder) MaxDelay(md time.Duration) StrategyBuilder { return eb } -// BaseDelay helps implement StrategyBuilder. -func (eb *exponentialBuilder) MinConnectTimeout(mct time.Duration) StrategyBuilder { - eb.mct = mct - return eb -} - // BaseDelay helps implement StrategyBuilder. func (eb *exponentialBuilder) Build() Strategy { return Exponential{ - baseDelay: eb.bd, - multiplier: eb.mltpr, - jitter: eb.jitter, - maxDelay: eb.md, - minConnectTimeout: eb.mct, + baseDelay: eb.bd, + multiplier: eb.mltpr, + jitter: eb.jitter, + maxDelay: eb.md, } } @@ -148,9 +133,6 @@ type Exponential struct { jitter float64 // maxDelay is the upper bound of backoff delay. maxDelay time.Duration - // minConnectTimeout is the minimum amount of time we are willing to give a - // connection to complete. - minConnectTimeout time.Duration } // Backoff returns the amount of time to wait before the next retry given the @@ -176,10 +158,3 @@ func (bc Exponential) Backoff(retries int) time.Duration { } return time.Duration(backoff) } - -// MinConnectionTimeout returns the minimum amount of time we are willing to -// give a connection to complete. -// Helps implement Strategy. -func (bc Exponential) MinConnectionTimeout() time.Duration { - return bc.minConnectTimeout -} diff --git a/internal/backoff/backoff_test.go b/internal/backoff/backoff_test.go index 78ff1ed53d0f..d88b9de2df5e 100644 --- a/internal/backoff/backoff_test.go +++ b/internal/backoff/backoff_test.go @@ -26,11 +26,10 @@ import ( func TestNewExponentialBuilderDefault(t *testing.T) { gotExp := NewExponentialBuilder().Build().(Strategy) wantExp := Exponential{ - baseDelay: baseDelay, - multiplier: multiplier, - jitter: jitter, - maxDelay: maxDelay, - minConnectTimeout: minConnectTimeout, + baseDelay: baseDelay, + multiplier: multiplier, + jitter: jitter, + maxDelay: maxDelay, } if gotExp != wantExp { t.Errorf("DefaultExponentialBuilder got: %v, want %v", gotExp, wantExp) @@ -42,11 +41,10 @@ func TestNewExponentialBuilderOverride(t *testing.T) { mltpr := 2.0 gotExp := NewExponentialBuilder().BaseDelay(bd).Multiplier(mltpr).Build().(Strategy) wantExp := Exponential{ - baseDelay: bd, - multiplier: mltpr, - jitter: jitter, - maxDelay: maxDelay, - minConnectTimeout: minConnectTimeout, + baseDelay: bd, + multiplier: mltpr, + jitter: jitter, + maxDelay: maxDelay, } if gotExp != wantExp { t.Errorf("OverrideExponentialBuilder got: %v, want %v", gotExp, wantExp) From 44d0c79b3e25eac7ca4d97b09573a123f46f4305 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Thu, 11 Apr 2019 17:27:50 -0700 Subject: [PATCH 4/9] Handle vet failure. --- internal/backoff/backoff.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go index 4d15ce7d7c99..7df806c7eea6 100644 --- a/internal/backoff/backoff.go +++ b/internal/backoff/backoff.go @@ -83,7 +83,6 @@ type exponentialBuilder struct { mltpr float64 jitter float64 md time.Duration - mct time.Duration } // BaseDelay helps implement StrategyBuilder. From 8ffe677584e516c87c7205892ac01ed6bf087164 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Tue, 14 May 2019 16:15:27 -0700 Subject: [PATCH 5/9] Rollback changes made as part of #2735. I need to roll these back because I had sent out changes from my master branch, and this is causing a lot of pain right now for my work on other branches. I will create a branch for this issue and will send out these changes in a fresh PR. --- backoff.go | 23 ------- balancer/grpclb/grpclb.go | 8 ++- balancer/xds/xds_client.go | 8 ++- clientconn.go | 8 ++- clientconn_test.go | 34 ++++------ dialoptions.go | 32 ++------- health/client.go | 4 +- internal/backoff/backoff.go | 109 ++++--------------------------- internal/backoff/backoff_test.go | 52 --------------- resolver/dns/dns_resolver.go | 4 +- 10 files changed, 55 insertions(+), 227 deletions(-) delete mode 100644 internal/backoff/backoff_test.go diff --git a/backoff.go b/backoff.go index d9de4b9f83fe..97c6e2568f45 100644 --- a/backoff.go +++ b/backoff.go @@ -27,35 +27,12 @@ import ( // DefaultBackoffConfig uses values specified for backoff in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. -// Deprecated: use ConnectParams instead. var DefaultBackoffConfig = BackoffConfig{ MaxDelay: 120 * time.Second, } // BackoffConfig defines the parameters for the default gRPC backoff strategy. -// Deprecated: use ConnectParams instead. type BackoffConfig struct { // MaxDelay is the upper bound of backoff delay. MaxDelay time.Duration } - -// ConnectParams defines the parameters for connecting and retrying. Users -// are encouraged to use this instead of BackoffConfig. -// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. -// -// This API is EXPERIMENTAL. -type ConnectParams struct { - // BackoffBaseDelay is the amount of time to backoff after the first - // connection failure. - BackoffBaseDelay time.Duration - // BackoffMultiplier is the factor with which to multiply backoffs after a - // failed retry. Should ideally be greater than 1. - BackoffMultiplier float64 - // BackoffJitter is the factor with which backoffs are randomized. - BackoffJitter float64 - // BackoffMaxDelay is the upper bound of backoff delay. - BackoffMaxDelay time.Duration - // MinConnectTimeout is the minimum amount of time we are willing to give a - // connection to complete. - MinConnectTimeout time.Duration -} diff --git a/balancer/grpclb/grpclb.go b/balancer/grpclb/grpclb.go index d1a0f32ecea4..a1123cedc7f8 100644 --- a/balancer/grpclb/grpclb.go +++ b/balancer/grpclb/grpclb.go @@ -51,14 +51,16 @@ const ( ) var ( - // defaultBackoffStrategy configures the backoff strategy that's used when the + // defaultBackoffConfig configures the backoff strategy that's used when the // init handshake in the RPC is unsuccessful. It's not for the clientconn // reconnect backoff. // // It has the same value as the default grpc.DefaultBackoffConfig. // // TODO: make backoff configurable. - defaultBackoffStrategy = backoff.NewExponentialBuilder().Build() + defaultBackoffConfig = backoff.Exponential{ + MaxDelay: 120 * time.Second, + } errServerTerminatedConnection = errors.New("grpclb: failed to recv server list: server terminated connection") ) @@ -174,7 +176,7 @@ func (b *lbBuilder) Build(cc balancer.ClientConn, opt balancer.BuildOptions) bal scStates: make(map[balancer.SubConn]connectivity.State), picker: &errPicker{err: balancer.ErrNoSubConnAvailable}, clientStats: newRPCStats(), - backoff: defaultBackoffStrategy, // TODO: make backoff configurable. + backoff: defaultBackoffConfig, // TODO: make backoff configurable. } var err error diff --git a/balancer/xds/xds_client.go b/balancer/xds/xds_client.go index 8b95a19a42b5..181bc3827209 100644 --- a/balancer/xds/xds_client.go +++ b/balancer/xds/xds_client.go @@ -44,7 +44,11 @@ const ( endpointRequired = "endpoints_required" ) -var defaultBackoffStrategy = backoff.NewExponentialBuilder().Build() +var ( + defaultBackoffConfig = backoff.Exponential{ + MaxDelay: 120 * time.Second, + } +) // client is responsible for connecting to the specified traffic director, passing the received // ADS response from the traffic director, and sending notification when communication with the @@ -253,7 +257,7 @@ func newXDSClient(balancerName string, serviceName string, enableCDS bool, opts newADS: newADS, loseContact: loseContact, cleanup: exitCleanup, - backoff: defaultBackoffStrategy, + backoff: defaultBackoffConfig, } c.ctx, c.cancel = context.WithCancel(context.Background()) diff --git a/clientconn.go b/clientconn.go index 1739bf83d001..bd2d2b317798 100644 --- a/clientconn.go +++ b/clientconn.go @@ -49,6 +49,8 @@ import ( ) const ( + // minimum time to give a connection to complete + minConnectTimeout = 20 * time.Second // must match grpclbName in grpclb/grpclb.go grpclbName = "grpclb" ) @@ -230,7 +232,9 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * } } if cc.dopts.bs == nil { - cc.dopts.bs = backoff.NewExponentialBuilder().Build() + cc.dopts.bs = backoff.Exponential{ + MaxDelay: DefaultBackoffConfig.MaxDelay, + } } if cc.dopts.resolverBuilder == nil { // Only try to parse target when resolver builder is not already set. @@ -950,7 +954,7 @@ func (ac *addrConn) resetTransport() { addrs := ac.addrs backoffFor := ac.dopts.bs.Backoff(ac.backoffIdx) // This will be the duration that dial gets to finish. - dialDuration := ac.dopts.mct + dialDuration := minConnectTimeout if ac.dopts.minConnectTimeout != nil { dialDuration = ac.dopts.minConnectTimeout() } diff --git a/clientconn_test.go b/clientconn_test.go index 464a0f3849ed..fa317971013e 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -820,31 +820,22 @@ func (s) TestCredentialsMisuse(t *testing.T) { } func (s) TestWithBackoffConfigDefault(t *testing.T) { - testBackoffStrategySet(t, backoff.NewExponentialBuilder().Build()) + testBackoffConfigSet(t, &DefaultBackoffConfig) } func (s) TestWithBackoffConfig(t *testing.T) { - md := DefaultBackoffConfig.MaxDelay / 2 - expected := backoff.NewExponentialBuilder().MaxDelay(md).Build() - testBackoffStrategySet(t, expected, WithBackoffConfig(BackoffConfig{MaxDelay: md})) + b := BackoffConfig{MaxDelay: DefaultBackoffConfig.MaxDelay / 2} + expected := b + testBackoffConfigSet(t, &expected, WithBackoffConfig(b)) } func (s) TestWithBackoffMaxDelay(t *testing.T) { md := DefaultBackoffConfig.MaxDelay / 2 - expected := backoff.NewExponentialBuilder().MaxDelay(md).Build() - testBackoffStrategySet(t, expected, WithBackoffMaxDelay(md)) -} - -func (s) TestWithConnectParams(t *testing.T) { - bd := 2 * time.Second - mltpr := 2.0 - jitter := 0.0 - crt := ConnectParams{BackoffBaseDelay: bd, BackoffMultiplier: mltpr, BackoffJitter: jitter} - expected := backoff.NewExponentialBuilder().BaseDelay(bd).Multiplier(mltpr).Jitter(jitter).MaxDelay(time.Duration(0)).Build() - testBackoffStrategySet(t, expected, WithConnectParams(crt)) + expected := BackoffConfig{MaxDelay: md} + testBackoffConfigSet(t, &expected, WithBackoffMaxDelay(md)) } -func testBackoffStrategySet(t *testing.T, expected backoff.Strategy, opts ...DialOption) { +func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOption) { opts = append(opts, WithInsecure()) conn, err := Dial("passthrough:///foo:80", opts...) if err != nil { @@ -853,16 +844,19 @@ func testBackoffStrategySet(t *testing.T, expected backoff.Strategy, opts ...Dia defer conn.Close() if conn.dopts.bs == nil { - t.Fatalf("backoff strategy not set") + t.Fatalf("backoff config not set") } actual, ok := conn.dopts.bs.(backoff.Exponential) if !ok { - t.Fatalf("unexpected type of backoff strategy: %#v", conn.dopts.bs) + t.Fatalf("unexpected type of backoff config : %#v", conn.dopts.bs) } - if actual != expected.(backoff.Exponential) { - t.Errorf("unexpected backoff strategy on connection: %v, want %v", actual, expected) + expectedValue := backoff.Exponential{ + MaxDelay: expected.MaxDelay, + } + if actual != expectedValue { + t.Fatalf("unexpected backoff config on connection: %v, want %v", actual, expected) } } diff --git a/dialoptions.go b/dialoptions.go index ee108f2c7b81..e114fecbb7b4 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -36,11 +36,6 @@ import ( "google.golang.org/grpc/stats" ) -const ( - // Minimum time to give a connection to complete. - minConnectTimeout = 20 * time.Second -) - // dialOptions configure a Dial call. dialOptions are set by the DialOption // values passed to Dial. type dialOptions struct { @@ -70,7 +65,6 @@ type dialOptions struct { minConnectTimeout func() time.Duration defaultServiceConfig *ServiceConfig // defaultServiceConfig is parsed from defaultServiceConfigRawJSON. defaultServiceConfigRawJSON *string - mct time.Duration } // DialOption configures how we set up the connection. @@ -255,26 +249,8 @@ func WithServiceConfig(c <-chan ServiceConfig) DialOption { }) } -// WithConnectParams configures the dialer to use the provided backoff -// parameters for the algorithm defined in -// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. -// This will override all the default values with the ones provided here. So, -// use with caution. -// -// This API is EXPERIMENTAL. -func WithConnectParams(p ConnectParams) DialOption { - b := backoff.NewExponentialBuilder() - b.BaseDelay(p.BackoffBaseDelay).Multiplier(p.BackoffMultiplier).Jitter(p.BackoffJitter).MaxDelay(p.BackoffMaxDelay) - return newFuncDialOption(func(o *dialOptions) { - o.bs = b.Build() - o.mct = p.MinConnectTimeout - }) -} - // WithBackoffMaxDelay configures the dialer to use the provided maximum delay // when backing off after failed connection attempts. -// -// Deprecated: use WithConnectParams instead. func WithBackoffMaxDelay(md time.Duration) DialOption { return WithBackoffConfig(BackoffConfig{MaxDelay: md}) } @@ -282,9 +258,12 @@ func WithBackoffMaxDelay(md time.Duration) DialOption { // WithBackoffConfig configures the dialer to use the provided backoff // parameters after connection failures. // -// Deprecated: use WithConnectParams instead. +// Use WithBackoffMaxDelay until more parameters on BackoffConfig are opened up +// for use. func WithBackoffConfig(b BackoffConfig) DialOption { - return withBackoff(backoff.NewExponentialBuilder().MaxDelay(b.MaxDelay).Build()) + return withBackoff(backoff.Exponential{ + MaxDelay: b.MaxDelay, + }) } // withBackoff sets the backoff strategy used for connectRetryNum after a failed @@ -538,7 +517,6 @@ func defaultDialOptions() dialOptions { WriteBufferSize: defaultWriteBufSize, ReadBufferSize: defaultReadBufSize, }, - mct: minConnectTimeout, } } diff --git a/health/client.go b/health/client.go index 73c5a6690611..e15f04c229c4 100644 --- a/health/client.go +++ b/health/client.go @@ -32,7 +32,9 @@ import ( "google.golang.org/grpc/status" ) -var backoffStrategy = backoff.NewExponentialBuilder().Build() +const maxDelay = 120 * time.Second + +var backoffStrategy = backoff.Exponential{MaxDelay: maxDelay} var backoffFunc = func(ctx context.Context, retries int) bool { d := backoffStrategy.Backoff(retries) timer := time.NewTimer(d) diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go index 7df806c7eea6..203d3dc6fdde 100644 --- a/internal/backoff/backoff.go +++ b/internal/backoff/backoff.go @@ -30,120 +30,39 @@ import ( // Strategy defines the methodology for backing off after a grpc connection // failure. +// type Strategy interface { // Backoff returns the amount of time to wait before the next retry given // the number of consecutive failures. Backoff(retries int) time.Duration } -// StrategyBuilder helps build an implementation of the Strategy interface, -// with setters provided to set individual parameters of the backoff strategy. -type StrategyBuilder interface { - // BaseDelay sets the amount of time to backoff after the first connection - // failure. - BaseDelay(time.Duration) StrategyBuilder - // Multiplier sets the the factor with which to multiply backoffs after a - // failed retry. - Multiplier(float64) StrategyBuilder - // Jitter sets the factor with which backoffs are randomized. - Jitter(float64) StrategyBuilder - // MaxDelay sets the upper bound of backoff delay. - MaxDelay(time.Duration) StrategyBuilder - // Build builds an implementation of the Strategy interface configured using - // the above setter methods. - Build() Strategy -} - -// Default values for the different backoff parameters as defined in -// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. const ( - baseDelay = 1.0 * time.Second - multiplier = 1.6 - jitter = 0.2 - maxDelay = 120 * time.Second + // baseDelay is the amount of time to wait before retrying after the first + // failure. + baseDelay = 1.0 * time.Second + // factor is applied to the backoff after each retry. + factor = 1.6 + // jitter provides a range to randomize backoff delays. + jitter = 0.2 ) -// NewExponentialBuilder returns an implementation of the StrategyBuilder -// interface used to build an exponential backoff strategy implementation, as -// defined in -// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. -func NewExponentialBuilder() StrategyBuilder { - return &exponentialBuilder{ - bd: baseDelay, - mltpr: multiplier, - jitter: jitter, - md: maxDelay, - } -} - -// exponentialBuilder implements the StrategyBuilder interface for an -// exponential backoff strategy. -type exponentialBuilder struct { - bd time.Duration - mltpr float64 - jitter float64 - md time.Duration -} - -// BaseDelay helps implement StrategyBuilder. -func (eb *exponentialBuilder) BaseDelay(bd time.Duration) StrategyBuilder { - eb.bd = bd - return eb -} - -// BaseDelay helps implement StrategyBuilder. -func (eb *exponentialBuilder) Multiplier(m float64) StrategyBuilder { - eb.mltpr = m - return eb -} - -// BaseDelay helps implement StrategyBuilder. -func (eb *exponentialBuilder) Jitter(j float64) StrategyBuilder { - eb.jitter = j - return eb -} - -// BaseDelay helps implement StrategyBuilder. -func (eb *exponentialBuilder) MaxDelay(md time.Duration) StrategyBuilder { - eb.md = md - return eb -} - -// BaseDelay helps implement StrategyBuilder. -func (eb *exponentialBuilder) Build() Strategy { - return Exponential{ - baseDelay: eb.bd, - multiplier: eb.mltpr, - jitter: eb.jitter, - maxDelay: eb.md, - } -} - // Exponential implements exponential backoff algorithm as defined in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. type Exponential struct { - // baseDelay is the amount of time to backoff after the first connection - // failure. - baseDelay time.Duration - // multiplier is the factor with which to multiply backoffs after a failed - // retry. - multiplier float64 - // jitter is the factor with which backoffs are randomized. - jitter float64 - // maxDelay is the upper bound of backoff delay. - maxDelay time.Duration + // MaxDelay is the upper bound of backoff delay. + MaxDelay time.Duration } // Backoff returns the amount of time to wait before the next retry given the // number of retries. -// Helps implement Strategy. func (bc Exponential) Backoff(retries int) time.Duration { if retries == 0 { - return bc.baseDelay + return baseDelay } - backoff, max := float64(bc.baseDelay), float64(bc.maxDelay) + backoff, max := float64(baseDelay), float64(maxDelay) for backoff < max && retries > 0 { - backoff *= bc.multiplier + backoff *= factor retries-- } if backoff > max { @@ -151,7 +70,7 @@ func (bc Exponential) Backoff(retries int) time.Duration { } // Randomize backoff delays so that if a cluster of requests start at // the same time, they won't operate in lockstep. - backoff *= 1 + bc.jitter*(grpcrand.Float64()*2-1) + backoff *= 1 + jitter*(grpcrand.Float64()*2-1) if backoff < 0 { return 0 } diff --git a/internal/backoff/backoff_test.go b/internal/backoff/backoff_test.go deleted file mode 100644 index d88b9de2df5e..000000000000 --- a/internal/backoff/backoff_test.go +++ /dev/null @@ -1,52 +0,0 @@ -/* - * - * Copyright 2019 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package backoff - -import ( - "testing" - "time" -) - -func TestNewExponentialBuilderDefault(t *testing.T) { - gotExp := NewExponentialBuilder().Build().(Strategy) - wantExp := Exponential{ - baseDelay: baseDelay, - multiplier: multiplier, - jitter: jitter, - maxDelay: maxDelay, - } - if gotExp != wantExp { - t.Errorf("DefaultExponentialBuilder got: %v, want %v", gotExp, wantExp) - } -} - -func TestNewExponentialBuilderOverride(t *testing.T) { - bd := 2 * time.Second - mltpr := 2.0 - gotExp := NewExponentialBuilder().BaseDelay(bd).Multiplier(mltpr).Build().(Strategy) - wantExp := Exponential{ - baseDelay: bd, - multiplier: mltpr, - jitter: jitter, - maxDelay: maxDelay, - } - if gotExp != wantExp { - t.Errorf("OverrideExponentialBuilder got: %v, want %v", gotExp, wantExp) - } -} diff --git a/resolver/dns/dns_resolver.go b/resolver/dns/dns_resolver.go index 3122eddb0f22..58355990779b 100644 --- a/resolver/dns/dns_resolver.go +++ b/resolver/dns/dns_resolver.go @@ -125,7 +125,7 @@ func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts ctx, cancel := context.WithCancel(context.Background()) d := &dnsResolver{ freq: b.minFreq, - backoff: backoff.NewExponentialBuilder().MaxDelay(b.minFreq).Build(), + backoff: backoff.Exponential{MaxDelay: b.minFreq}, host: host, port: port, ctx: ctx, @@ -197,7 +197,7 @@ func (i *ipResolver) watcher() { // dnsResolver watches for the name resolution update for a non-IP target. type dnsResolver struct { freq time.Duration - backoff backoff.Strategy + backoff backoff.Exponential retryCount int host string port string From 1c29ad5d2f2b322b628e4fe009699412994fd0d5 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Tue, 14 May 2019 16:27:40 -0700 Subject: [PATCH 6/9] Missed this in the previous commit. --- internal/backoff/backoff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go index 203d3dc6fdde..1bd0cce5abdf 100644 --- a/internal/backoff/backoff.go +++ b/internal/backoff/backoff.go @@ -60,7 +60,7 @@ func (bc Exponential) Backoff(retries int) time.Duration { if retries == 0 { return baseDelay } - backoff, max := float64(baseDelay), float64(maxDelay) + backoff, max := float64(baseDelay), float64(bc.MaxDelay) for backoff < max && retries > 0 { backoff *= factor retries-- From 57e1ed6ed208fc48bf9fc8a4269b8974675a4093 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Thu, 16 May 2019 13:17:10 -0700 Subject: [PATCH 7/9] Unexpected diff from upstream. --- clientconn_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clientconn_test.go b/clientconn_test.go index fa317971013e..356f4f25af95 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -849,7 +849,7 @@ func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOpt actual, ok := conn.dopts.bs.(backoff.Exponential) if !ok { - t.Fatalf("unexpected type of backoff config : %#v", conn.dopts.bs) + t.Fatalf("unexpected type of backoff config: %#v", conn.dopts.bs) } expectedValue := backoff.Exponential{ From d6bf849e514eda4d81c632019aea6b88d9930ebf Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Thu, 16 May 2019 15:06:49 -0700 Subject: [PATCH 8/9] Support for backoff params as defined in https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. fixes #2724 --- backoff.go | 25 +++++++ balancer/grpclb/grpclb.go | 8 +-- balancer/xds/lrs/lrs.go | 4 +- balancer/xds/xds_client.go | 8 +-- clientconn.go | 8 +-- clientconn_test.go | 34 ++++++---- dialoptions.go | 32 +++++++-- health/client.go | 4 +- internal/backoff/backoff.go | 109 +++++++++++++++++++++++++++---- internal/backoff/backoff_test.go | 52 +++++++++++++++ resolver/dns/dns_resolver.go | 4 +- 11 files changed, 230 insertions(+), 58 deletions(-) create mode 100644 internal/backoff/backoff_test.go diff --git a/backoff.go b/backoff.go index 97c6e2568f45..675fcd92c2c6 100644 --- a/backoff.go +++ b/backoff.go @@ -27,12 +27,37 @@ import ( // DefaultBackoffConfig uses values specified for backoff in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +// +// Deprecated: use ConnectParams instead. var DefaultBackoffConfig = BackoffConfig{ MaxDelay: 120 * time.Second, } // BackoffConfig defines the parameters for the default gRPC backoff strategy. +// +// Deprecated: use ConnectParams instead. type BackoffConfig struct { // MaxDelay is the upper bound of backoff delay. MaxDelay time.Duration } + +// ConnectParams defines the parameters for connecting and retrying. Users +// are encouraged to use this instead of BackoffConfig. +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +// +// This API is EXPERIMENTAL. +type ConnectParams struct { + // BackoffBaseDelay is the amount of time to backoff after the first + // connection failure. + BackoffBaseDelay time.Duration + // BackoffMultiplier is the factor with which to multiply backoffs after a + // failed retry. Should ideally be greater than 1. + BackoffMultiplier float64 + // BackoffJitter is the factor with which backoffs are randomized. + BackoffJitter float64 + // BackoffMaxDelay is the upper bound of backoff delay. + BackoffMaxDelay time.Duration + // MinConnectTimeout is the minimum amount of time we are willing to give a + // connection to complete. + MinConnectTimeout time.Duration +} diff --git a/balancer/grpclb/grpclb.go b/balancer/grpclb/grpclb.go index cc1f0ec9ecd7..c404bd9f8792 100644 --- a/balancer/grpclb/grpclb.go +++ b/balancer/grpclb/grpclb.go @@ -50,16 +50,14 @@ const ( ) var ( - // defaultBackoffConfig configures the backoff strategy that's used when the + // defaultBackoffStrategy configures the backoff strategy that's used when the // init handshake in the RPC is unsuccessful. It's not for the clientconn // reconnect backoff. // // It has the same value as the default grpc.DefaultBackoffConfig. // // TODO: make backoff configurable. - defaultBackoffConfig = backoff.Exponential{ - MaxDelay: 120 * time.Second, - } + defaultBackoffStrategy = backoff.NewExponentialBuilder().Build() errServerTerminatedConnection = errors.New("grpclb: failed to recv server list: server terminated connection") ) @@ -155,7 +153,7 @@ func (b *lbBuilder) Build(cc balancer.ClientConn, opt balancer.BuildOptions) bal scStates: make(map[balancer.SubConn]connectivity.State), picker: &errPicker{err: balancer.ErrNoSubConnAvailable}, clientStats: newRPCStats(), - backoff: defaultBackoffConfig, // TODO: make backoff configurable. + backoff: defaultBackoffStrategy, // TODO: make backoff configurable. } var err error diff --git a/balancer/xds/lrs/lrs.go b/balancer/xds/lrs/lrs.go index acac1adf7e86..30b370dfb660 100644 --- a/balancer/xds/lrs/lrs.go +++ b/balancer/xds/lrs/lrs.go @@ -66,9 +66,7 @@ func NewStore(serviceName string) Store { }, }, }, - backoff: backoff.Exponential{ - MaxDelay: 120 * time.Second, - }, + backoff: backoff.NewExponentialBuilder().Build(), lastReported: time.Now(), } } diff --git a/balancer/xds/xds_client.go b/balancer/xds/xds_client.go index ef779ff8c034..bc98123371c1 100644 --- a/balancer/xds/xds_client.go +++ b/balancer/xds/xds_client.go @@ -48,11 +48,7 @@ const ( endpointRequired = "endpoints_required" ) -var ( - defaultBackoffConfig = backoff.Exponential{ - MaxDelay: 120 * time.Second, - } -) +var defaultBackoffStrategy = backoff.NewExponentialBuilder().Build() // client is responsible for connecting to the specified traffic director, passing the received // ADS response from the traffic director, and sending notification when communication with the @@ -277,7 +273,7 @@ func newXDSClient(balancerName string, enableCDS bool, opts balancer.BuildOption newADS: newADS, loseContact: loseContact, cleanup: exitCleanup, - backoff: defaultBackoffConfig, + backoff: defaultBackoffStrategy, loadStore: loadStore, } diff --git a/clientconn.go b/clientconn.go index 78e6d178a1b4..e57b6239310c 100644 --- a/clientconn.go +++ b/clientconn.go @@ -49,8 +49,6 @@ import ( ) const ( - // minimum time to give a connection to complete - minConnectTimeout = 20 * time.Second // must match grpclbName in grpclb/grpclb.go grpclbName = "grpclb" ) @@ -235,9 +233,7 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn * } } if cc.dopts.bs == nil { - cc.dopts.bs = backoff.Exponential{ - MaxDelay: DefaultBackoffConfig.MaxDelay, - } + cc.dopts.bs = backoff.NewExponentialBuilder().Build() } if cc.dopts.resolverBuilder == nil { // Only try to parse target when resolver builder is not already set. @@ -1021,7 +1017,7 @@ func (ac *addrConn) resetTransport() { addrs := ac.addrs backoffFor := ac.dopts.bs.Backoff(ac.backoffIdx) // This will be the duration that dial gets to finish. - dialDuration := minConnectTimeout + dialDuration := ac.dopts.mct if ac.dopts.minConnectTimeout != nil { dialDuration = ac.dopts.minConnectTimeout() } diff --git a/clientconn_test.go b/clientconn_test.go index 356f4f25af95..7ff25cc58c8b 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -820,22 +820,31 @@ func (s) TestCredentialsMisuse(t *testing.T) { } func (s) TestWithBackoffConfigDefault(t *testing.T) { - testBackoffConfigSet(t, &DefaultBackoffConfig) + testBackoffStrategySet(t, backoff.NewExponentialBuilder().Build()) } func (s) TestWithBackoffConfig(t *testing.T) { - b := BackoffConfig{MaxDelay: DefaultBackoffConfig.MaxDelay / 2} - expected := b - testBackoffConfigSet(t, &expected, WithBackoffConfig(b)) + md := DefaultBackoffConfig.MaxDelay / 2 + expected := backoff.NewExponentialBuilder().MaxDelay(md).Build() + testBackoffStrategySet(t, expected, WithBackoffConfig(BackoffConfig{MaxDelay: md})) } func (s) TestWithBackoffMaxDelay(t *testing.T) { md := DefaultBackoffConfig.MaxDelay / 2 - expected := BackoffConfig{MaxDelay: md} - testBackoffConfigSet(t, &expected, WithBackoffMaxDelay(md)) + expected := backoff.NewExponentialBuilder().MaxDelay(md).Build() + testBackoffStrategySet(t, expected, WithBackoffMaxDelay(md)) +} + +func (s) TestWithConnectParams(t *testing.T) { + bd := 2 * time.Second + mltpr := 2.0 + jitter := 0.0 + crt := ConnectParams{BackoffBaseDelay: bd, BackoffMultiplier: mltpr, BackoffJitter: jitter} + expected := backoff.NewExponentialBuilder().BaseDelay(bd).Multiplier(mltpr).Jitter(jitter).MaxDelay(time.Duration(0)).Build() + testBackoffStrategySet(t, expected, WithConnectParams(crt)) } -func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOption) { +func testBackoffStrategySet(t *testing.T, expected *backoff.Strategy, opts ...DialOption) { opts = append(opts, WithInsecure()) conn, err := Dial("passthrough:///foo:80", opts...) if err != nil { @@ -844,19 +853,16 @@ func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOpt defer conn.Close() if conn.dopts.bs == nil { - t.Fatalf("backoff config not set") + t.Fatalf("backoff strategy not set") } actual, ok := conn.dopts.bs.(backoff.Exponential) if !ok { - t.Fatalf("unexpected type of backoff config: %#v", conn.dopts.bs) + t.Fatalf("unexpected type of backoff strategy: %#v", conn.dopts.bs) } - expectedValue := backoff.Exponential{ - MaxDelay: expected.MaxDelay, - } - if actual != expectedValue { - t.Fatalf("unexpected backoff config on connection: %v, want %v", actual, expected) + if actual != expected.(backoff.Exponential) { + t.Errorf("unexpected backoff strategy on connection: %v, want %v", actual, expected) } } diff --git a/dialoptions.go b/dialoptions.go index 1fdc619d1730..e83e3d013392 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -36,6 +36,11 @@ import ( "google.golang.org/grpc/stats" ) +const ( + // Minimum time to give a connection to complete. + minConnectTimeout = 20 * time.Second +) + // dialOptions configure a Dial call. dialOptions are set by the DialOption // values passed to Dial. type dialOptions struct { @@ -69,6 +74,7 @@ type dialOptions struct { minConnectTimeout func() time.Duration defaultServiceConfig *ServiceConfig // defaultServiceConfig is parsed from defaultServiceConfigRawJSON. defaultServiceConfigRawJSON *string + mct time.Duration } // DialOption configures how we set up the connection. @@ -253,8 +259,26 @@ func WithServiceConfig(c <-chan ServiceConfig) DialOption { }) } +// WithConnectParams configures the dialer to use the provided backoff +// parameters for the algorithm defined in +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +// This will override all the default values with the ones provided here. So, +// use with caution. +// +// This API is EXPERIMENTAL. +func WithConnectParams(p ConnectParams) DialOption { + b := backoff.NewExponentialBuilder() + b.BaseDelay(p.BackoffBaseDelay).Multiplier(p.BackoffMultiplier).Jitter(p.BackoffJitter).MaxDelay(p.BackoffMaxDelay) + return newFuncDialOption(func(o *dialOptions) { + o.bs = b.Build() + o.mct = p.MinConnectTimeout + }) +} + // WithBackoffMaxDelay configures the dialer to use the provided maximum delay // when backing off after failed connection attempts. +// +// Deprecated: use WithConnectParams instead. func WithBackoffMaxDelay(md time.Duration) DialOption { return WithBackoffConfig(BackoffConfig{MaxDelay: md}) } @@ -262,12 +286,9 @@ func WithBackoffMaxDelay(md time.Duration) DialOption { // WithBackoffConfig configures the dialer to use the provided backoff // parameters after connection failures. // -// Use WithBackoffMaxDelay until more parameters on BackoffConfig are opened up -// for use. +// Deprecated: use WithConnectParams instead. func WithBackoffConfig(b BackoffConfig) DialOption { - return withBackoff(backoff.Exponential{ - MaxDelay: b.MaxDelay, - }) + return withBackoff(backoff.NewExponentialBuilder().MaxDelay(b.MaxDelay).Build()) } // withBackoff sets the backoff strategy used for connectRetryNum after a failed @@ -543,6 +564,7 @@ func defaultDialOptions() dialOptions { WriteBufferSize: defaultWriteBufSize, ReadBufferSize: defaultReadBufSize, }, + mct: minConnectTimeout, } } diff --git a/health/client.go b/health/client.go index e15f04c229c4..73c5a6690611 100644 --- a/health/client.go +++ b/health/client.go @@ -32,9 +32,7 @@ import ( "google.golang.org/grpc/status" ) -const maxDelay = 120 * time.Second - -var backoffStrategy = backoff.Exponential{MaxDelay: maxDelay} +var backoffStrategy = backoff.NewExponentialBuilder().Build() var backoffFunc = func(ctx context.Context, retries int) bool { d := backoffStrategy.Backoff(retries) timer := time.NewTimer(d) diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go index 1bd0cce5abdf..7df806c7eea6 100644 --- a/internal/backoff/backoff.go +++ b/internal/backoff/backoff.go @@ -30,39 +30,120 @@ import ( // Strategy defines the methodology for backing off after a grpc connection // failure. -// type Strategy interface { // Backoff returns the amount of time to wait before the next retry given // the number of consecutive failures. Backoff(retries int) time.Duration } -const ( - // baseDelay is the amount of time to wait before retrying after the first +// StrategyBuilder helps build an implementation of the Strategy interface, +// with setters provided to set individual parameters of the backoff strategy. +type StrategyBuilder interface { + // BaseDelay sets the amount of time to backoff after the first connection // failure. - baseDelay = 1.0 * time.Second - // factor is applied to the backoff after each retry. - factor = 1.6 - // jitter provides a range to randomize backoff delays. - jitter = 0.2 + BaseDelay(time.Duration) StrategyBuilder + // Multiplier sets the the factor with which to multiply backoffs after a + // failed retry. + Multiplier(float64) StrategyBuilder + // Jitter sets the factor with which backoffs are randomized. + Jitter(float64) StrategyBuilder + // MaxDelay sets the upper bound of backoff delay. + MaxDelay(time.Duration) StrategyBuilder + // Build builds an implementation of the Strategy interface configured using + // the above setter methods. + Build() Strategy +} + +// Default values for the different backoff parameters as defined in +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +const ( + baseDelay = 1.0 * time.Second + multiplier = 1.6 + jitter = 0.2 + maxDelay = 120 * time.Second ) +// NewExponentialBuilder returns an implementation of the StrategyBuilder +// interface used to build an exponential backoff strategy implementation, as +// defined in +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +func NewExponentialBuilder() StrategyBuilder { + return &exponentialBuilder{ + bd: baseDelay, + mltpr: multiplier, + jitter: jitter, + md: maxDelay, + } +} + +// exponentialBuilder implements the StrategyBuilder interface for an +// exponential backoff strategy. +type exponentialBuilder struct { + bd time.Duration + mltpr float64 + jitter float64 + md time.Duration +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) BaseDelay(bd time.Duration) StrategyBuilder { + eb.bd = bd + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) Multiplier(m float64) StrategyBuilder { + eb.mltpr = m + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) Jitter(j float64) StrategyBuilder { + eb.jitter = j + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) MaxDelay(md time.Duration) StrategyBuilder { + eb.md = md + return eb +} + +// BaseDelay helps implement StrategyBuilder. +func (eb *exponentialBuilder) Build() Strategy { + return Exponential{ + baseDelay: eb.bd, + multiplier: eb.mltpr, + jitter: eb.jitter, + maxDelay: eb.md, + } +} + // Exponential implements exponential backoff algorithm as defined in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. type Exponential struct { - // MaxDelay is the upper bound of backoff delay. - MaxDelay time.Duration + // baseDelay is the amount of time to backoff after the first connection + // failure. + baseDelay time.Duration + // multiplier is the factor with which to multiply backoffs after a failed + // retry. + multiplier float64 + // jitter is the factor with which backoffs are randomized. + jitter float64 + // maxDelay is the upper bound of backoff delay. + maxDelay time.Duration } // Backoff returns the amount of time to wait before the next retry given the // number of retries. +// Helps implement Strategy. func (bc Exponential) Backoff(retries int) time.Duration { if retries == 0 { - return baseDelay + return bc.baseDelay } - backoff, max := float64(baseDelay), float64(bc.MaxDelay) + backoff, max := float64(bc.baseDelay), float64(bc.maxDelay) for backoff < max && retries > 0 { - backoff *= factor + backoff *= bc.multiplier retries-- } if backoff > max { @@ -70,7 +151,7 @@ func (bc Exponential) Backoff(retries int) time.Duration { } // Randomize backoff delays so that if a cluster of requests start at // the same time, they won't operate in lockstep. - backoff *= 1 + jitter*(grpcrand.Float64()*2-1) + backoff *= 1 + bc.jitter*(grpcrand.Float64()*2-1) if backoff < 0 { return 0 } diff --git a/internal/backoff/backoff_test.go b/internal/backoff/backoff_test.go new file mode 100644 index 000000000000..d88b9de2df5e --- /dev/null +++ b/internal/backoff/backoff_test.go @@ -0,0 +1,52 @@ +/* + * + * Copyright 2019 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package backoff + +import ( + "testing" + "time" +) + +func TestNewExponentialBuilderDefault(t *testing.T) { + gotExp := NewExponentialBuilder().Build().(Strategy) + wantExp := Exponential{ + baseDelay: baseDelay, + multiplier: multiplier, + jitter: jitter, + maxDelay: maxDelay, + } + if gotExp != wantExp { + t.Errorf("DefaultExponentialBuilder got: %v, want %v", gotExp, wantExp) + } +} + +func TestNewExponentialBuilderOverride(t *testing.T) { + bd := 2 * time.Second + mltpr := 2.0 + gotExp := NewExponentialBuilder().BaseDelay(bd).Multiplier(mltpr).Build().(Strategy) + wantExp := Exponential{ + baseDelay: bd, + multiplier: mltpr, + jitter: jitter, + maxDelay: maxDelay, + } + if gotExp != wantExp { + t.Errorf("OverrideExponentialBuilder got: %v, want %v", gotExp, wantExp) + } +} diff --git a/resolver/dns/dns_resolver.go b/resolver/dns/dns_resolver.go index 297492e87af4..0a9dfe8f3bb7 100644 --- a/resolver/dns/dns_resolver.go +++ b/resolver/dns/dns_resolver.go @@ -128,7 +128,7 @@ func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts ctx, cancel := context.WithCancel(context.Background()) d := &dnsResolver{ freq: b.minFreq, - backoff: backoff.Exponential{MaxDelay: b.minFreq}, + backoff: backoff.NewExponentialBuilder().MaxDelay(b.minFreq).Build(), host: host, port: port, ctx: ctx, @@ -200,7 +200,7 @@ func (i *ipResolver) watcher() { // dnsResolver watches for the name resolution update for a non-IP target. type dnsResolver struct { freq time.Duration - backoff backoff.Exponential + backoff backoff.Strategy retryCount int host string port string From c91f66385984136198e3801aa10fd87fada236b4 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Thu, 16 May 2019 16:40:02 -0700 Subject: [PATCH 9/9] Overlooked a * when making the original commit. --- clientconn_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clientconn_test.go b/clientconn_test.go index 7ff25cc58c8b..464a0f3849ed 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -844,7 +844,7 @@ func (s) TestWithConnectParams(t *testing.T) { testBackoffStrategySet(t, expected, WithConnectParams(crt)) } -func testBackoffStrategySet(t *testing.T, expected *backoff.Strategy, opts ...DialOption) { +func testBackoffStrategySet(t *testing.T, expected backoff.Strategy, opts ...DialOption) { opts = append(opts, WithInsecure()) conn, err := Dial("passthrough:///foo:80", opts...) if err != nil {