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

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

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,
+}

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,26 +661,30 @@ 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))
 }
 
 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

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

internal/backoff/backoff.go

@@ -25,69 +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
 }
 
-// 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 {
 		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 + bc.Jitter*(grpcrand.Float64()*2-1)
+	backoff *= 1 + bc.Config.Jitter*(grpcrand.Float64()*2-1)
 	if backoff < 0 {
 		return 0
 	}

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,