Implement missing pieces for connection backoff. #2821
Conversation
| // interface used to build an exponential backoff strategy implementation, as | ||
| // defined in | ||
| // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. | ||
| func NewExponentialBuilder() StrategyBuilder { |
There was a problem hiding this comment.
Go doesn't typically have builders or getters/setters. Just structs with fields.
Why not simply export the fields in Exponential?
Defaults could be handled by something like:
package backoff
var DefaultExponential = Exponential{<defaults>}
...
package grpc
s := DefaultExponential
s.BaseDelay = 10*time.Secondor
Make all the fields pointers and handle nil pointers in the Backoff function. This is a bit more painful to use and handle, though you could make it easy to use with helpers like func DurationPtr(d time.Duration) *time.Duration { return &d }.
| @@ -69,6 +74,7 @@ type dialOptions struct { | |||
| minConnectTimeout func() time.Duration | |||
| defaultServiceConfig *ServiceConfig // defaultServiceConfig is parsed from defaultServiceConfigRawJSON. | |||
| defaultServiceConfigRawJSON *string | |||
| mct time.Duration | |||
There was a problem hiding this comment.
There is a minConnectTimeout func() time.Duration above - this would be duplicating that.
Also, in general: try not to use short names for things outside a small scope (i.e. a short function or a struct with 1-2 fields where it's entirely obvious what the short name means (e.g. timer.C)).
| // | ||
| // This API is EXPERIMENTAL. | ||
| type ConnectParams struct { | ||
| // BackoffBaseDelay is the amount of time to backoff after the first |
There was a problem hiding this comment.
Could we replace all the backoff fields here with a single BackoffConfig field, and add these fields to the BackoffConfig (excluding MinConnectTimeout)? Theoretically we could pass BackoffConfig to Exponential directly, though that is unfortunately not easy because it's in a different package and has circular dependency issues. We'd need a new, non-internal backoff package. This may actually be a good idea.
Move MinConnectionTimeout() out of backoff.Strategy interface. Store it directly in dialOptions instead, and have ClientConn use it from there.
I need to roll these back because I had sent out changes from my master branch, and this is causing a lot of pain right now for my work on other branches. I will create a branch for this issue and will send out these changes in a fresh PR.
Move MinConnectionTimeout() out of backoff.Strategy interface. Store it directly in dialOptions instead, and have ClientConn use it from there.
I need to roll these back because I had sent out changes from my master branch, and this is causing a lot of pain right now for my work on other branches. I will create a branch for this issue and will send out these changes in a fresh PR.
|
Tracked in #2960. |
These are the same set of changes initially proposed in #2735.
#2724