Repository navigation
feat(dns): retry requests on 408, 502, 503, 504 - #1854
cgoetz-inovex wants to merge 2 commits into
Conversation
| } | ||
|
|
||
| func (f *DefaultClientFactory) defaultConfigOptions(customEndpoint string) []config.ConfigurationOption { | ||
| func (f *DefaultClientFactory) defaultConfigOptions(customEndpoint string, additionalRetryStatusCodes ...int) []config.ConfigurationOption { |
There was a problem hiding this comment.
Went for just status code varargs instead of function opts varargs, since this will probably become the default in 2 weeks
| } | ||
| } | ||
|
|
||
| func TestRetryTransport_AdditionalStatusCodes_RetryAfter(t *testing.T) { |
There was a problem hiding this comment.
Retry-After checked for all status codes. The standard only gives some examples:
https://httpwg.org/specs/rfc9110.html#field.retry-after
| MaxRetries: defaultMaxRetries, | ||
| BaseBackoff: defaultBaseBackoff, | ||
| MaxJitter: defaultMaxJitter, // Always added to wait time | ||
| AdditionalRetryStatusCodes: additionalRetryStatusCodes, |
There was a problem hiding this comment.
Could we please have a note here that this is not intended for GA yet and only for selected services?
There was a problem hiding this comment.
I think we also had it that way for the HTTP 429 retries back then.
| http.StatusGatewayTimeout, | ||
| } | ||
|
|
||
| for _, statusCode := range statusCodes { |
There was a problem hiding this comment.
I guess it makes sense to have this as a table test which asserts that e.g. HTTP 200 status codes are not retried while the ones currently listed are.
There was a problem hiding this comment.
non-retries of other status codes is covered in TestDefaultClientFactory_RetryRequests. Since this state is rather short lived, I've opted to only test the additional behaviour here in separate tests.
When this retry behaviour gets applied to the default middlewares, we can delete the tests added in this PR and extend the ones for the default middleware stack.
Is this ok for you or should I replicate some test cases here?
Description
relates to #1234
Checklist
make fmtexamples/directory)make generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)