-
Notifications
You must be signed in to change notification settings - Fork 10.2k
clientv3: implement exponential backoff mechanism #20731
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,12 @@ var ( | |
|
|
||
| // client-side retry backoff default jitter fraction. | ||
| defaultBackoffJitterFraction = 0.10 | ||
|
|
||
| // client-side retry backoff exponential factor. Default of 1.0 which is no exponential backoff. | ||
| defaultBackoffExponent = 1.0 | ||
|
|
||
| // client-side retry backoff exponential max wait between requests. | ||
| defaultBackoffMaxWaitBetween = 5 * time.Second | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please give some context on why 5 seconds?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I chose 5 seconds as it is roughly in line with other widely used client side libraries. For example the aws-sdk-go-v2 client libraries use a default max backoff of 20 seconds: https://github.com/aws/aws-sdk-go-v2/blob/main/aws/retry/standard.go#L31 Also, from experience a backoff in the range of 5-30 seconds is long enough to load shed, but short enough that system will converge in a timely manner. This is only from my experience running distributed systems. I tried to research if there are any white papers evaluating max backoff parameters, but I could not find any. If someone has a strong reason why the default max backoff should be a different value I would be amenable to changing it. |
||
| ) | ||
|
|
||
| // defaultCallOpts defines a list of default "gRPC.CallOption". | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| // Copyright 2025 The etcd 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 clientv3 | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "math" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestExpBackoff(t *testing.T) { | ||
| testCases := []struct { | ||
| generation uint | ||
| exponent float64 | ||
| minDelay time.Duration | ||
| maxDelay time.Duration | ||
| expectedBackoff time.Duration | ||
| }{ | ||
| // exponential backoff with 2.0 exponent | ||
| {generation: 0, exponent: 2.0, minDelay: 100 * time.Millisecond, maxDelay: 500 * time.Millisecond, expectedBackoff: 100 * time.Millisecond}, | ||
| {generation: 1, exponent: 2.0, minDelay: 100 * time.Millisecond, maxDelay: 500 * time.Millisecond, expectedBackoff: 200 * time.Millisecond}, | ||
| {generation: 2, exponent: 2.0, minDelay: 100 * time.Millisecond, maxDelay: 500 * time.Millisecond, expectedBackoff: 400 * time.Millisecond}, | ||
| {generation: 3, exponent: 2.0, minDelay: 100 * time.Millisecond, maxDelay: 500 * time.Millisecond, expectedBackoff: 500 * time.Millisecond}, | ||
| {generation: math.MaxUint, exponent: 2.0, minDelay: 100 * time.Millisecond, maxDelay: 500 * time.Millisecond, expectedBackoff: 500 * time.Millisecond}, | ||
|
|
||
| // exponential backoff with 1.0 exponent | ||
| {generation: 0, exponent: 1.0, minDelay: 100 * time.Millisecond, maxDelay: 500 * time.Millisecond, expectedBackoff: 100 * time.Millisecond}, | ||
| {generation: 1, exponent: 1.0, minDelay: 100 * time.Millisecond, maxDelay: 500 * time.Millisecond, expectedBackoff: 100 * time.Millisecond}, | ||
| {generation: 2, exponent: 1.0, minDelay: 100 * time.Millisecond, maxDelay: 500 * time.Millisecond, expectedBackoff: 100 * time.Millisecond}, | ||
| {generation: 3, exponent: 1.0, minDelay: 100 * time.Millisecond, maxDelay: 500 * time.Millisecond, expectedBackoff: 100 * time.Millisecond}, | ||
| {generation: math.MaxUint, exponent: 1.0, minDelay: 100 * time.Millisecond, maxDelay: 500 * time.Millisecond, expectedBackoff: 100 * time.Millisecond}, | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was also thinking if it is worth having a testcase for backoffExponent = 0 and/or exponent = 0: Regarding this function: func expBackoff(generation uint, exponent float64, minDelay, maxDelay time.Duration) time.Duration {
delay := math.Min(math.Pow(exponent, float64(generation))*float64(minDelay), float64(maxDelay))
return time.Duration(delay)
}
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is not a possible configuration since if the user passes in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. |
||
|
|
||
| for _, testCase := range testCases { | ||
| testName := fmt.Sprintf("%+v", testCase) | ||
| t.Run(testName, func(t *testing.T) { | ||
| backoff := expBackoff(testCase.generation, testCase.exponent, testCase.minDelay, testCase.maxDelay) | ||
| require.InDelta(t, testCase.expectedBackoff, backoff, 0.01) | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add another testcase with backoffExponent = 1.0 to ensure that reverse compatibility is disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, added a test case for backoffExponent = 1.0