From 000fdc7a85a5ebf2bf9b35df91543ce7bea9b11d Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Fri, 30 Aug 2019 01:56:10 +0000 Subject: [PATCH] Handle review comments. Added a new backoff package which defines the backoff configuration options, and is used by both the grpc package and the internal/backoff package. This allows us to have all backoff related options in a separate package. --- backoff.go | 22 +++++++-------- backoff/backoff.go | 53 ++++++++++++++++++++++++++++++++++++ clientconn_test.go | 31 +++++++++++++++++---- dialoptions.go | 23 +++++++--------- internal/backoff/backoff.go | 38 ++++++-------------------- resolver/dns/dns_resolver.go | 7 +++-- 6 files changed, 110 insertions(+), 64 deletions(-) create mode 100644 backoff/backoff.go diff --git a/backoff.go b/backoff.go index 4849c51ea2ae..af58980a51f3 100644 --- a/backoff.go +++ b/backoff.go @@ -23,6 +23,8 @@ package grpc import ( "time" + + grpcbackoff "google.golang.org/grpc/backoff" ) // DefaultBackoffConfig uses values specified for backoff in @@ -41,20 +43,16 @@ type BackoffConfig struct { MaxDelay time.Duration } -// ConnectParams defines the parameters for connecting and retrying. 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 the type defined above. See +// here for more details: // 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 + // Backoff specifies the configuration options for connection backoff. + Backoff grpcbackoff.Config + // MinConnectTimeout is the minimum amount of time we are willing to give a + // connection to complete. + MinConnectTimeout time.Duration } diff --git a/backoff/backoff.go b/backoff/backoff.go new file mode 100644 index 000000000000..d59b3e60802f --- /dev/null +++ b/backoff/backoff.go @@ -0,0 +1,53 @@ +/* + * + * 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 provides configuration options for connection backoff. +package backoff + +import "time" + +// Config defines the configuration options for connection backoff. More +// details can be found at +// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +// +// This API is EXPERIMENTAL. +type Config struct { + // BaseDelay is the amount of time to backoff after the first failure. + BaseDelay time.Duration + // Multiplier is the factor with which to multiply backoffs after a + // failed retry. Should ideally be greater than 1. + Multiplier float64 + // Jitter is the factor with which backoffs are randomized. + Jitter float64 + // MaxDelay is the upper bound of backoff delay. + MaxDelay time.Duration +} + +// DefaultConfig is a backoff configuration with the default values specfied +// in https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. +// +// This should be useful for callers who want to configure backoff with +// non-default values only for a subset of the options. +// +// This API is EXPERIMENTAL. +var DefaultConfig = Config{ + BaseDelay: 1.0 * time.Second, + Multiplier: 1.6, + Jitter: 0.2, + MaxDelay: 120 * time.Second, +} diff --git a/clientconn_test.go b/clientconn_test.go index 58f8356b86c0..ee6ed454d0d4 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -30,6 +30,7 @@ import ( "time" "golang.org/x/net/http2" + grpcbackoff "google.golang.org/grpc/backoff" "google.golang.org/grpc/connectivity" "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal/backoff" @@ -660,15 +661,17 @@ func (s) TestWithBackoffConfigDefault(t *testing.T) { func (s) TestWithBackoffConfig(t *testing.T) { b := BackoffConfig{MaxDelay: DefaultBackoffConfig.MaxDelay / 2} - wantBackoff := backoff.DefaultExponential - wantBackoff.MaxDelay = b.MaxDelay + bc := grpcbackoff.DefaultConfig + bc.MaxDelay = b.MaxDelay + wantBackoff := backoff.Exponential{bc} testBackoffConfigSet(t, wantBackoff, WithBackoffConfig(b)) } func (s) TestWithBackoffMaxDelay(t *testing.T) { md := DefaultBackoffConfig.MaxDelay / 2 - wantBackoff := backoff.DefaultExponential - wantBackoff.MaxDelay = md + bc := grpcbackoff.DefaultConfig + bc.MaxDelay = md + wantBackoff := backoff.Exponential{bc} testBackoffConfigSet(t, wantBackoff, WithBackoffMaxDelay(md)) } @@ -676,10 +679,12 @@ func (s) TestWithConnectParams(t *testing.T) { bd := 2 * time.Second mltpr := 2.0 jitter := 0.0 - crt := ConnectParams{BackoffBaseDelay: bd, BackoffMultiplier: mltpr, BackoffJitter: jitter} + bc := grpcbackoff.Config{BaseDelay: bd, Multiplier: mltpr, Jitter: jitter} + + crt := ConnectParams{Backoff: bc} // MaxDelay is not set in the ConnectParams. So it should not be set on // backoff.Exponential as well. - wantBackoff := backoff.Exponential{BaseDelay: bd, Multiplier: mltpr, Jitter: jitter} + wantBackoff := backoff.Exponential{bc} testBackoffConfigSet(t, wantBackoff, WithConnectParams(crt)) } @@ -705,6 +710,20 @@ func testBackoffConfigSet(t *testing.T, wantBackoff backoff.Exponential, opts .. } } +func (s) TestConnectParamsWithMinConnectTimeout(t *testing.T) { + // Default value specified for minConnectTimeout in the spec is 20 seconds. + mct := 1 * time.Minute + conn, err := Dial("passthrough:///foo:80", WithInsecure(), WithConnectParams(ConnectParams{MinConnectTimeout: mct})) + if err != nil { + t.Fatalf("unexpected error dialing connection: %v", err) + } + defer conn.Close() + + if got := conn.dopts.minConnectTimeout(); got != mct { + t.Errorf("unexpect minConnectTimeout on the connection: %v, want %v", got, mct) + } +} + // emptyBalancer returns an empty set of servers. type emptyBalancer struct { ch chan []Address diff --git a/dialoptions.go b/dialoptions.go index 162f9e52d424..5143fa207eef 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -24,6 +24,7 @@ import ( "net" "time" + grpcbackoff "google.golang.org/grpc/backoff" "google.golang.org/grpc/balancer" "google.golang.org/grpc/credentials" "google.golang.org/grpc/grpclog" @@ -246,19 +247,15 @@ 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. +// WithConnectParams configures the dialer to use the provided ConnectParams. // // This API is EXPERIMENTAL. func WithConnectParams(p ConnectParams) DialOption { - return withBackoff(backoff.Exponential{ - BaseDelay: p.BackoffBaseDelay, - Multiplier: p.BackoffMultiplier, - Jitter: p.BackoffJitter, - MaxDelay: p.BackoffMaxDelay, + return newFuncDialOption(func(o *dialOptions) { + o.bs = backoff.Exponential{p.Backoff} + o.minConnectTimeout = func() time.Duration { + return p.MinConnectTimeout + } }) } @@ -275,9 +272,9 @@ func WithBackoffMaxDelay(md time.Duration) DialOption { // //Deprecated: use WithConnectParams instead. func WithBackoffConfig(b BackoffConfig) DialOption { - bs := backoff.DefaultExponential - bs.MaxDelay = b.MaxDelay - return withBackoff(bs) + bc := grpcbackoff.DefaultConfig + bc.MaxDelay = b.MaxDelay + return withBackoff(backoff.Exponential{bc}) } // withBackoff sets the backoff strategy used for connectRetryNum after a failed diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go index 95c38ab8768d..5fc0ee3da53b 100644 --- a/internal/backoff/backoff.go +++ b/internal/backoff/backoff.go @@ -25,61 +25,39 @@ package backoff import ( "time" + grpcbackoff "google.golang.org/grpc/backoff" "google.golang.org/grpc/internal/grpcrand" ) // 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 } -// These constants correspond to the ones defined in -// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. -const ( - // 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 - // maxDelay is the upper bound of backoff delay. - maxDelay = 120 * time.Second -) - // DefaultExponential is an exponential backoff implementation using the // default values for all the configurable knobs defined in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. -var DefaultExponential = Exponential{BaseDelay: baseDelay, Multiplier: factor, Jitter: jitter, MaxDelay: maxDelay} +var DefaultExponential = Exponential{Config: grpcbackoff.DefaultConfig} // 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 + // Config contains all options to configure the backoff algorithm. + Config grpcbackoff.Config } // Backoff returns the amount of time to wait before the next retry given the // number of retries. func (bc Exponential) Backoff(retries int) time.Duration { if retries == 0 { - return bc.BaseDelay + return bc.Config.BaseDelay } - backoff, max := float64(bc.BaseDelay), float64(bc.MaxDelay) + backoff, max := float64(bc.Config.BaseDelay), float64(bc.Config.MaxDelay) for backoff < max && retries > 0 { - backoff *= bc.Multiplier + backoff *= bc.Config.Multiplier retries-- } if backoff > max { @@ -87,7 +65,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 + bc.Config.Jitter*(grpcrand.Float64()*2-1) if backoff < 0 { return 0 } diff --git a/resolver/dns/dns_resolver.go b/resolver/dns/dns_resolver.go index 151cd3924664..a52ea08e470a 100644 --- a/resolver/dns/dns_resolver.go +++ b/resolver/dns/dns_resolver.go @@ -32,6 +32,7 @@ import ( "sync" "time" + grpcbackoff "google.golang.org/grpc/backoff" "google.golang.org/grpc/grpclog" "google.golang.org/grpc/internal/backoff" "google.golang.org/grpc/internal/grpcrand" @@ -126,11 +127,11 @@ func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts // DNS address (non-IP). ctx, cancel := context.WithCancel(context.Background()) - bs := backoff.DefaultExponential - bs.MaxDelay = b.minFreq + bc := grpcbackoff.DefaultConfig + bc.MaxDelay = b.minFreq d := &dnsResolver{ freq: b.minFreq, - backoff: bs, + backoff: backoff.Exponential{bc}, host: host, port: port, ctx: ctx,