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

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Aug 6, 2019

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 easwars requested a review from dfawley August 6, 2019 22:29
@easwars
Copy link
Contributor Author

easwars commented Aug 6, 2019

@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 easwars changed the title Backoff new Implement missing pieces for connection backoff Aug 8, 2019
@dfawley dfawley self-assigned this Aug 15, 2019
@easwars
Copy link
Contributor Author

easwars commented Aug 22, 2019

Fixed the merge conflicts.

@linux-foundation-easycla
Copy link

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

@easwars
Copy link
Contributor Author

easwars commented Aug 30, 2019

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

@easwars
Copy link
Contributor Author

easwars commented Sep 30, 2019

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.
backoff.go Outdated
@@ -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.
Copy link
Contributor

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.

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

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

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.

backoff.go Outdated
@@ -23,16 +23,36 @@ package grpc

import (
"time"

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

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.

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

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
Contributor

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.

*
*/

// Package backoff provides configuration options for connection backoff.
Copy link
Contributor

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.

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

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

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.

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

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

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.

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

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

dialoptions.go Outdated
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
Contributor

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.

Copy link
Contributor Author

@easwars easwars left a comment

Choose a reason for hiding this comment

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

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

backoff.go Outdated
@@ -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.
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.

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

// BackoffConfig defines the parameters for the default gRPC backoff strategy.
//
// Deprecated: use ConnectParams 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.

backoff.go Outdated
@@ -23,16 +23,36 @@ package grpc

import (
"time"

grpcbackoff "google.golang.org/grpc/backoff"
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.

*
*/

// Package backoff provides configuration options for connection backoff.
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.

// details can be found at
// https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md.
//
// This API is EXPERIMENTAL.
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.

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

"golang.org/x/net/http2"
grpcbackoff "google.golang.org/grpc/backoff"
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.

dialoptions.go Outdated
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
Contributor Author

Choose a reason for hiding this comment

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

Done.

dialoptions.go Outdated
// WithBackoffMaxDelay configures the dialer to use the provided maximum delay
// when backing off after failed connection attempts.
//
//Deprecated: use WithConnectParams 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.

I don't see any strange character here :)

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

// WithConnectParams configures the dialer to use the provided ConnectParams.
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.

Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

does import renaming affect godoc

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

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

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.

@dfawley dfawley added this to the 1.25 Release milestone Oct 3, 2019
@dfawley dfawley changed the title Implement missing pieces for connection backoff client: add WithConnectParams to configure connection backoff and timeout Oct 3, 2019
@easwars easwars merged commit 31911ed into grpc:master Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More parameters for exponential connection backoff
2 participants