Support for configuring exponential connection backoff params #2735
Conversation
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
|
Hmm ... I've signed the CLA now as an individual and as an employee. Not sure if this gets updated automatically. |
Update fork with changes from master
backoff.go
Outdated
| // failure. | ||
| BaseDelay time.Duration | ||
| // Multiplier is the factor with which to multiply backoffs after a failed | ||
| // retry. |
There was a problem hiding this comment.
Any bounds on this? I.e. must be > 0? (Or > 1?)
There was a problem hiding this comment.
Added a comment saying it should be ideally greater than 1. Should we have validation code for that or more explicit warnings that if the user sets it to less than 1, they are shooting themselves in the foot?
internal/backoff/backoff.go
Outdated
| 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 | ||
| // MinConnectionTimeout returns the minimum amount of time we are willing to | ||
| // give a connection to complete. | ||
| MinConnectionTimeout() time.Duration |
There was a problem hiding this comment.
MinConnectTimeout is not a function of backoff, so it doesn't really belong in this package. I think we can keep MCT separate from backoff strategy and store them separately in the ClientConn.
There was a problem hiding this comment.
Removed it from here.
Added it as a field in dialOptions with defaultDialOptions() setting the existing default value for this. Overrides are done from WithConnectParams(), which feels a little odd to me, but thought I will run it by you anyways.
| // baseDelay is the amount of time to wait before retrying after the first | ||
| // StrategyBuilder helps build an implementation of the Strategy interface, | ||
| // with setters provided to set individual parameters of the backoff strategy. | ||
| type StrategyBuilder interface { |
There was a problem hiding this comment.
This is not very Go-idiomatic. Why not export the fields in Exponential and set them directly? This is an internal package, so we can do whatever we want with it w.r.t. changing how the defaults are applied.
There was a problem hiding this comment.
Yes, if the fields of Exponential are exposed, we can directly set them from WithConnectParams() where all fields are expected.
But in places where only a subset of the fields are to be modified and the rest should continue to have their default values (especially for other internal packages), this is much easier to use. Also, it makes it easier to allow setting any of the fields to their zero value.
Move MinConnectionTimeout() out of backoff.Strategy interface. Store it directly in dialOptions instead, and have ClientConn use it from there.
|
PTAL. |
|
This was the first PR that I sent, and I had sent this out of my master branch (just poor git skills). And now this is turning out to be pain as I create new branches to work on other things. So, I'm going to close this, and will send out a fresh one from a new branch. Thanks. |
|
Note that there is nothing special about your "master" branch. It's the same as any other branch. You just needed to create a different branch in your fork based off grpc's master. Something like this (there may be even better ways, but this is the limit of my git knowledge): |
|
(Also sorry for the delays in reviewing.) |
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.
|
Thank you! |
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.
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.
Support for backoff params as defined in https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md.
fixes #2724