Internal API for retrying HTTP requests #518
Conversation
| @@ -166,8 +166,66 @@ export class HttpError extends Error { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Specifies how failing HTTP requests should be retried. | |||
There was a problem hiding this comment.
Please explain what the different fields mean in this interface (an example would be helpful). For example maxDelayInMillis is per retry and not in total per request, etc. backoff factor in seconds, etc.
| return this.sendWithRetry(config, attempts + 1); | ||
| }) | ||
| .catch((err: LowLevelError) => { | ||
| const [delayMillis, canRetry] = this.getRetryDelayMillis(retryAttempts, err); |
There was a problem hiding this comment.
The private functions added do not provide enough descriptions on what is being computed or a summary of the underlying logic making it harder to deduce the underlying behavior. It helps to either add more comments here or descriptions of the private functions below.
There was a problem hiding this comment.
I think getRetryDelayMillis() is the confusing one, although I feel that the variable names explain what's going on. In any case, I've added a jsdoc comment to the method to further explain it.
test/unit/utils/api-request.spec.ts
Outdated
| }).should.eventually.be.rejectedWith(err).and.have.property('code', 'app/network-error'); | ||
| }); | ||
|
|
||
| it('should not retry when for error codes that are not configured', () => { |
There was a problem hiding this comment.
should not retry when error codes are not configured
|
|
||
| private backOffDelayMillis(retryAttempts: number): number { | ||
| if (retryAttempts === 0) { | ||
| return 0; |
There was a problem hiding this comment.
Why do you not wait on the first retry?
You will end up with a wait pattern: first fail, 0, 2000, 4000
Instead of: first fail, 1000, 2000, 4000
I think the latter has a better likelihood of succeeding with less time overall.
There was a problem hiding this comment.
This is to retain the existing behavior, and also to align with how retries are implemented in other languages/libraries. Basically we only delay retries in the event of consecutive errors. Here's a similar implementation from Python's urllib3 package: https://github.com/urllib3/urllib3/blob/master/src/urllib3/util/retry.py#L222
In general, the existing strategy of immediately retrying on the first error has worked well for us so far. This is particularly useful in environments like GCF where low-level transient errors seem to be common.
test/unit/utils/api-request.spec.ts
Outdated
| expect(resp.data).to.deep.equal(respData); | ||
| expect(resp.isJson()).to.be.true; | ||
| expect(delayStub.callCount).to.equal(1); | ||
| expect(delayStub.args[0][0]).to.be.gt(27 * 1000).and.to.be.lte(30 * 1000); |
There was a problem hiding this comment.
How was 27 * 1000 determined?
There was a problem hiding this comment.
Managed to make the assertion exact by using sinon fake timers.
|
Thanks @bojeil-google. Made most of the suggested changes. Ready for another look. |
src/utils/api-request.ts
Outdated
| /** Maximum number of times to retry a given request. */ | ||
| maxRetries: number; | ||
|
|
||
| /** HTTP status codes taht should be retried. */ |
src/utils/api-request.ts
Outdated
| ioErrorCodes?: string[]; | ||
|
|
||
| /** | ||
| * The multiplier for exponential back off. The retry delay is calculated using the formula `(2^n) * backOffFactor`, |
There was a problem hiding this comment.
Clarify that it's in seconds
The retry delay is calculated using the formula (2^n) * backOffFactor seconds,
The existing
HttpClientretries all HTTP requests once that are failing due to connection timeout and reset errors. We would like to further extend this retries support to meet the following new requirements:Retry-Afterheader sent by the serverIn order to meet all the above requirements, this PR introduces a new internal interface called
RetryConfig. TheHttpClientoptionally takes aRetryConfigas a constructor parameter, and retries failing requests based on that.For the moment, we use the following as the default
RetryConfigin order to retain the existing behavior:We can change this to something more sophisticated in the future (in other Admin SDKs we retry up to 4 times, and also retry on 500 and 503 HTTP errors).