refactor: Unify precondition retry logic#1660
Conversation
| * - if precondition option `ifGenerationMatch` is not set OR | ||
| * - if `idempotencyStrategy` is set to `RetryNever` | ||
| */ | ||
| private shouldRetry(options: RetryOptions): boolean { |
There was a problem hiding this comment.
maybe work into the title that this is in regards to precondition stuff? we already have another function similarly named for error code retry logic
| preconditionOpts?: PreconditionOptions; | ||
| } | ||
|
|
||
| export interface RetryOptions { |
There was a problem hiding this comment.
i think this is also misnamed. is this interface needed? can the shouldRetry function take in PreconditionOptions directly?
There was a problem hiding this comment.
I can see the value in splitting out this interface if we were going to be doing the etag work soon but for the moment I would agree with Sameena that shouldRetry can probably just take PreconditionOptions directly.
| this.storage.retryOptions.idempotencyStrategy === | ||
| IdempotencyStrategy.RetryNever | ||
| ) { | ||
| if (!this.shouldRetryRequest(options.preconditionOpts)) { |
There was a problem hiding this comment.
will this throw if options is undefined?
There was a problem hiding this comment.
options is defined on line 1035
| * - if precondition option `ifGenerationMatch` is not set OR | ||
| * - if `idempotencyStrategy` is set to `RetryNever` | ||
| */ | ||
| private shouldRetryRequest(options?: PreconditionOptions): boolean { |
There was a problem hiding this comment.
can you please rename to shouldRetryBasedOnPreconditionAndIdempotencyStrat or maybe something less long but still descriptive? we already have too many functions that effectively decide shouldRetryRequest
- in common: https://github.com/googleapis/nodejs-common/blob/6d239d3b8c58c1e00571135e1e715e4c5cd8c2a3/src/util.ts#L545
- in storage:
Line 260 in 5ede98d
…ionAndIdempotencyStrat`
| * A helper method for determining if a request should be retried based on preconditions | ||
| * | ||
| * A request should not be retried under the following conditions: | ||
| * - if precondition option `ifGenerationMatch` is not set OR |
There was a problem hiding this comment.
If I understand correctly, this should only be used for methods where the idempotency is determined by ifGenerationMatch, not for ones where metageneration is used-- is that correct? If so we should make that clear.
There was a problem hiding this comment.
Good point - I'll make a note. Perhaps we can extend the function when we require IfMetagenerationMatch checking in this class.
Refs:
We can extend the function when we require `IfMetagenerationMatch` checking in this class
…nto unify-retry-logic
No description provided.