Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: add WithConnectParams to configure connection backoff and timeout #2960

Merged
merged 10 commits into from Oct 3, 2019
20 changes: 20 additions & 0 deletions backoff.go
Expand Up @@ -23,16 +23,36 @@ package grpc

import (
"time"

grpcbackoff "google.golang.org/grpc/backoff"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to rename in this file. I'm worried a renaming here might affect godoc for the ConnectParams field (? not sure about it though).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Removed unnecessary import renaming.

Hmm .... does import renaming affect godoc? I couldn't find any documentation to that effect.

)

// DefaultBackoffConfig uses values specified for backoff in
// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md.
//
// Deprecated: use ConnectParams instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify that this will be supported throughout 1.x (copy/paste similar deprecation comments elsewhere in grpc package).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

var DefaultBackoffConfig = BackoffConfig{
MaxDelay: 120 * time.Second,
}

// BackoffConfig defines the parameters for the default gRPC backoff strategy.
//
// Deprecated: use ConnectParams instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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 the BackoffConfig 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 {
// 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
}
53 changes: 53 additions & 0 deletions 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/connection//? This can be (and is/will be) used for other things as well as connection backoffs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this comment to the whole package level instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually referring to the "EXPERIMENTAL" part. Please mark the whole file as experimental. Following the same wording as resolver/resolver.go:

// All APIs in this package are 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
// at 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,
}
16 changes: 2 additions & 14 deletions balancer/grpclb/grpclb.go
Expand Up @@ -49,19 +49,7 @@ const (
grpclbName = "grpclb"
)

var (
// 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.
defaultBackoffConfig = backoff.Exponential{
MaxDelay: 120 * time.Second,
}
errServerTerminatedConnection = errors.New("grpclb: failed to recv server list: server terminated connection")
)
var errServerTerminatedConnection = errors.New("grpclb: failed to recv server list: server terminated connection")

func convertDuration(d *durationpb.Duration) time.Duration {
if d == nil {
Expand Down Expand Up @@ -155,7 +143,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: backoff.DefaultExponential, // TODO: make backoff configurable.
}

var err error
Expand Down
4 changes: 1 addition & 3 deletions clientconn.go
Expand Up @@ -235,9 +235,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.DefaultExponential
}
if cc.dopts.resolverBuilder == nil {
// Only try to parse target when resolver builder is not already set.
Expand Down
51 changes: 40 additions & 11 deletions clientconn_test.go
Expand Up @@ -30,6 +30,7 @@ import (
"time"

"golang.org/x/net/http2"
grpcbackoff "google.golang.org/grpc/backoff"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to nit-pick, but I would prefer to rename internal/backoff and leave "main" packages like this not renamed where possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"google.golang.org/grpc/connectivity"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/internal/backoff"
Expand Down Expand Up @@ -655,22 +656,39 @@ func (s) TestCredentialsMisuse(t *testing.T) {
}

func (s) TestWithBackoffConfigDefault(t *testing.T) {
testBackoffConfigSet(t, &DefaultBackoffConfig)
testBackoffConfigSet(t, backoff.DefaultExponential)
}

func (s) TestWithBackoffConfig(t *testing.T) {
b := BackoffConfig{MaxDelay: DefaultBackoffConfig.MaxDelay / 2}
expected := b
testBackoffConfigSet(t, &expected, WithBackoffConfig(b))
bc := grpcbackoff.DefaultConfig
bc.MaxDelay = b.MaxDelay
wantBackoff := backoff.Exponential{Config: bc}
testBackoffConfigSet(t, wantBackoff, WithBackoffConfig(b))
}

func (s) TestWithBackoffMaxDelay(t *testing.T) {
md := DefaultBackoffConfig.MaxDelay / 2
expected := BackoffConfig{MaxDelay: md}
testBackoffConfigSet(t, &expected, WithBackoffMaxDelay(md))
bc := grpcbackoff.DefaultConfig
bc.MaxDelay = md
wantBackoff := backoff.Exponential{Config: bc}
testBackoffConfigSet(t, wantBackoff, WithBackoffMaxDelay(md))
}

func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOption) {
func (s) TestWithConnectParams(t *testing.T) {
bd := 2 * time.Second
mltpr := 2.0
jitter := 0.0
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{Config: bc}
testBackoffConfigSet(t, wantBackoff, WithConnectParams(crt))
}

func testBackoffConfigSet(t *testing.T, wantBackoff backoff.Exponential, opts ...DialOption) {
opts = append(opts, WithInsecure())
conn, err := Dial("passthrough:///foo:80", opts...)
if err != nil {
Expand All @@ -682,16 +700,27 @@ func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOpt
t.Fatalf("backoff config not set")
}

actual, ok := conn.dopts.bs.(backoff.Exponential)
gotBackoff, ok := conn.dopts.bs.(backoff.Exponential)
if !ok {
t.Fatalf("unexpected type of backoff config: %#v", conn.dopts.bs)
}

expectedValue := backoff.Exponential{
MaxDelay: expected.MaxDelay,
if gotBackoff != wantBackoff {
t.Fatalf("unexpected backoff config on connection: %v, want %v", gotBackoff, wantBackoff)
}
if actual != expectedValue {
t.Fatalf("unexpected backoff config on connection: %v, want %v", actual, expected)
}

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

Expand Down
24 changes: 19 additions & 5 deletions dialoptions.go
Expand Up @@ -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"
Expand Down Expand Up @@ -246,21 +247,34 @@ func WithServiceConfig(c <-chan ServiceConfig) DialOption {
})
}

// WithConnectParams configures the dialer to use the provided ConnectParams.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the best place to document our defaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the comments a bit more verbose.

//
// This API is EXPERIMENTAL.
func WithConnectParams(p ConnectParams) DialOption {
return newFuncDialOption(func(o *dialOptions) {
o.bs = backoff.Exponential{Config: p.Backoff}
o.minConnectTimeout = func() time.Duration {
return p.MinConnectTimeout
}
})
}

// WithBackoffMaxDelay configures the dialer to use the provided maximum delay
// when backing off after failed connection attempts.
//
//Deprecated: use WithConnectParams instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange character (between // and Deprecated)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any strange character here :)

func WithBackoffMaxDelay(md time.Duration) DialOption {
return WithBackoffConfig(BackoffConfig{MaxDelay: md})
}

// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

func WithBackoffConfig(b BackoffConfig) DialOption {
return withBackoff(backoff.Exponential{
MaxDelay: b.MaxDelay,
})
bc := grpcbackoff.DefaultConfig
bc.MaxDelay = b.MaxDelay
return withBackoff(backoff.Exponential{Config: bc})
}

// withBackoff sets the backoff strategy used for connectRetryNum after a failed
Expand Down
26 changes: 13 additions & 13 deletions health/client.go
Expand Up @@ -33,20 +33,20 @@ import (
"google.golang.org/grpc/status"
)

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)
select {
case <-timer.C:
return true
case <-ctx.Done():
timer.Stop()
return false
var (
backoffStrategy = backoff.DefaultExponential
backoffFunc = func(ctx context.Context, retries int) bool {
d := backoffStrategy.Backoff(retries)
timer := time.NewTimer(d)
select {
case <-timer.C:
return true
case <-ctx.Done():
timer.Stop()
return false
}
}
}
)

func init() {
internal.HealthCheckFunc = clientHealthCheck
Expand Down
27 changes: 11 additions & 16 deletions internal/backoff/backoff.go
Expand Up @@ -25,52 +25,47 @@ 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
}

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
)
// 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{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 {
// 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 baseDelay
return bc.Config.BaseDelay
}
backoff, max := float64(baseDelay), float64(bc.MaxDelay)
backoff, max := float64(bc.Config.BaseDelay), float64(bc.Config.MaxDelay)
for backoff < max && retries > 0 {
backoff *= factor
backoff *= bc.Config.Multiplier
retries--
}
if backoff > max {
backoff = max
}
// 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.Config.Jitter*(grpcrand.Float64()*2-1)
if backoff < 0 {
return 0
}
Expand Down
5 changes: 4 additions & 1 deletion resolver/dns/dns_resolver.go
Expand Up @@ -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"
Expand Down Expand Up @@ -126,9 +127,11 @@ func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts

// DNS address (non-IP).
ctx, cancel := context.WithCancel(context.Background())
bc := grpcbackoff.DefaultConfig
bc.MaxDelay = b.minFreq
d := &dnsResolver{
freq: b.minFreq,
backoff: backoff.Exponential{MaxDelay: b.minFreq},
backoff: backoff.Exponential{Config: bc},
host: host,
port: port,
ctx: ctx,
Expand Down
4 changes: 1 addition & 3 deletions xds/internal/balancer/lrs/lrs.go
Expand Up @@ -161,9 +161,7 @@ func NewStore(serviceName string) Store {
},
},
},
backoff: backoff.Exponential{
MaxDelay: 120 * time.Second,
},
backoff: backoff.DefaultExponential,
lastReported: time.Now(),
}
}
Expand Down