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

easwars · 2019-08-06T22:29:43Z

Implement missing pieces for connection backoff.

Spec can be found here:
https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md

Summary of changes:
* Added a new type (marked experimental), ConnectParams, which contains
  the knobs defined in the spec (except for minConnectTimeout).
* Added a new API (marked experimental), WithConnectParams() to return a
  DialOption to dial with the provided parameters.
* Added new fields to the implementation of the exponential backoff in
  internal/backoff which mirror the ones in ConnectParams.
* Marked existing APIs WithBackoffMaxDelay() and WithBackoffConfig() as
  deprecated.
* Added a default exponential backoff implementation, for easy use of
  internal callers.

Fixes #2724

easwars · 2019-08-06T22:31:37Z

@dfawley
Sorry, I screwed up my previous branch. Hence a new PR.

Got rid of the builder pattern in Exponential and instead just exported the fields.
Got rid of the processing of minConnectionTimeout completely, since it already has a DialOption (although marked, for testing purposes only).

easwars · 2019-08-22T16:07:48Z

Fixed the merge conflicts.

linux-foundation-easycla · 2019-08-30T02:14:50Z

One or more committers are not authorized under a signed CLA as indicated below. Please click here to be authorized.

✅ Easwar Swaminathan (ab8d9e8, 1c683d2, 45bd284, 000fdc7, 097dd8e, 4d26f5e, 6bc792e, 3795dc5, fdab21d, 822bacc, e90fd41, a509d91)
✅ Srini Polavarapu (ad20eaf)
✅ Michael Hudson-Doyle (d5a36f0)
✅ James Protzman (4d39b48)

❌ Michael Le (0574097)

easwars · 2019-08-30T02:23:36Z

I have moved the backoff configuration parameters to a new package. PTAL.

easwars · 2019-09-30T21:53:27Z

Ping @dfawley

Spec can be found here: https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md Summary of changes: * Added a new type (marked experimental), ConnectParams, which contains the knobs defined in the spec (except for minConnectTimeout). * Added a new API (marked experimental), WithConnectParams() to return a DialOption to dial with the provided parameters. * Added new fields to the implementation of the exponential backoff in internal/backoff which mirror the ones in ConnectParams. * Marked existing APIs WithBackoffMaxDelay() and WithBackoffConfig() as deprecated. * Added a default exponential backoff implementation, for easy use of internal callers.

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.

dfawley · 2019-08-29T23:17:47Z

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 ConnectParams instead.


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

dfawley · 2019-08-29T23:17:51Z

backoff.go

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

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


dfawley · 2019-08-30T15:46:47Z

backoff.go

@@ -23,16 +23,36 @@ package grpc

 import (
 	"time"
+
+	grpcbackoff "google.golang.org/grpc/backoff"


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

Done. Removed unnecessary import renaming.

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

dfawley · 2019-08-30T15:51:34Z

backoff/backoff.go

+// details can be found at
+// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md.
+//
+// This API is EXPERIMENTAL.


Please move this comment to the whole package level instead.

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.

dfawley · 2019-08-30T15:53:06Z

backoff/backoff.go

+ *
+ */
+
+// Package backoff provides configuration options for connection backoff.


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

dfawley · 2019-10-01T21:08:17Z

clientconn_test.go

@@ -30,6 +30,7 @@ import (
 	"time"

 	"golang.org/x/net/http2"
+	grpcbackoff "google.golang.org/grpc/backoff"


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

dfawley · 2019-10-01T21:08:59Z

dialoptions.go

@@ -246,21 +247,34 @@ func WithServiceConfig(c <-chan ServiceConfig) DialOption {
 	})
 }

+// WithConnectParams configures the dialer to use the provided ConnectParams.


Is this the best place to document our defaults?

Made the comments a bit more verbose.

dfawley · 2019-10-01T21:09:37Z

dialoptions.go

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


Strange character (between // and Deprecated)?

I don't see any strange character here :)

dfawley · 2019-10-01T21:09:42Z

dialoptions.go

 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.


easwars

PTAL. Removed unnecessary import renaming wherever possible, and where required renamed the internal/backoff package instead of grpc/backoff package.

easwars · 2019-10-02T16:40:54Z

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 ConnectParams instead.


easwars · 2019-10-02T16:41:04Z

backoff.go

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

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


easwars · 2019-10-02T16:46:29Z

backoff.go

@@ -23,16 +23,36 @@ package grpc

 import (
 	"time"
+
+	grpcbackoff "google.golang.org/grpc/backoff"


Done. Removed unnecessary import renaming.

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

easwars · 2019-10-02T16:49:16Z

backoff/backoff.go

+ *
+ */
+
+// Package backoff provides configuration options for connection backoff.


easwars · 2019-10-02T16:49:26Z

backoff/backoff.go

+// details can be found at
+// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md.
+//
+// This API is EXPERIMENTAL.


easwars · 2019-10-02T17:07:38Z

clientconn_test.go

@@ -30,6 +30,7 @@ import (
 	"time"

 	"golang.org/x/net/http2"
+	grpcbackoff "google.golang.org/grpc/backoff"


easwars · 2019-10-02T17:11:26Z

dialoptions.go

 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.


easwars · 2019-10-02T17:12:11Z

dialoptions.go

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


I don't see any strange character here :)

easwars · 2019-10-02T17:26:32Z

dialoptions.go

@@ -246,21 +247,34 @@ func WithServiceConfig(c <-chan ServiceConfig) DialOption {
 	})
 }

+// WithConnectParams configures the dialer to use the provided ConnectParams.


Made the comments a bit more verbose.

dfawley

does import renaming affect godoc

I am not sure. I would hope not, but I also wouldn't be too surprised if it did.

dfawley · 2019-10-03T17:08:41Z

backoff/backoff.go

+// details can be found at
+// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md.
+//
+// This API is EXPERIMENTAL.


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.

easwars requested a review from dfawley August 6, 2019 22:29

easwars mentioned this pull request Aug 6, 2019

Implement missing pieces for connection backoff. #2821

Closed

easwars changed the title ~~Backoff new~~ Implement missing pieces for connection backoff Aug 8, 2019

dfawley self-assigned this Aug 15, 2019

easwars force-pushed the backoff_new branch from 345965c to 1c683d2 Compare August 22, 2019 16:03

easwars force-pushed the backoff_new branch from a509d91 to e90fd41 Compare August 30, 2019 02:18

easwars added 5 commits September 30, 2019 22:24

Add comment for exported var DefaultExponential.

f8b4003

Handle review comments.

0f7db76

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.

Make vet happy.

0115f00

vets and comments

dea6444

easwars force-pushed the backoff_new branch from e90fd41 to dea6444 Compare September 30, 2019 22:24

Missed during merge conflict resolution.

f5998d1

dfawley reviewed Oct 1, 2019

View reviewed changes

easwars added 2 commits October 2, 2019 17:36

Review comments.

3fce38e

Make vet happy.

e1a1470

easwars commented Oct 2, 2019

View reviewed changes

Change a weird character masquerading as a space.

ebebd7d

dfawley reviewed Oct 3, 2019

View reviewed changes

dfawley added the Type: Feature New features or improvements in behavior label Oct 3, 2019

dfawley added this to the 1.25 Release milestone Oct 3, 2019

dfawley changed the title ~~Implement missing pieces for connection backoff~~ client: add WithConnectParams to configure connection backoff and timeout Oct 3, 2019

Review comments.

cc7872c

dfawley approved these changes Oct 3, 2019

View reviewed changes

easwars merged commit 31911ed into grpc:master Oct 3, 2019

easwars deleted the backoff_new branch October 4, 2019 21:48

lysu mentioned this pull request Oct 16, 2019

tikv: Fix the issue that KV requests might be processed slower because the connections to some KV servers are slow to establish. pingcap/tidb#12733

Merged

lock bot locked as resolved and limited conversation to collaborators Apr 2, 2020

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

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

Conversation

easwars commented Aug 6, 2019 • edited by dfawley Loading

easwars commented Aug 6, 2019

easwars commented Aug 22, 2019

linux-foundation-easycla bot commented Aug 30, 2019

easwars commented Aug 30, 2019

easwars commented Sep 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Aug 6, 2019 •

edited by dfawley

Loading