use async-retry to not handle/redo the poll-timing logic #398
Conversation
| iteratee = (_report: ResourceSnapshotsReportModel) => {} | ||
| public waitUntilDone( | ||
| operationToWaitFor: ResourceSnapshotsReportType | null, | ||
| options: Partial<WaitUntilDoneOptions> = {}, |
There was a problem hiding this comment.
Given the spread l145, we can provide a Partial. This allow the caller to only set the options he want
| return retry( | ||
| this.waitUntilDoneRetryFunction(operationToWaitFor, onRetryCb), | ||
| { | ||
| retries: Infinity, |
There was a problem hiding this comment.
We don't care about the number of attempt, so Infinity
There was a problem hiding this comment.
Are you sure it supports Infinity? And that you don't have to use Number.MAX_SAFE_INTEGER
| minTimeout: toMilliseconds(opts.waitInterval), | ||
| maxTimeout: toMilliseconds(opts.waitInterval), |
There was a problem hiding this comment.
We want to retry every opts.waitInterval seconds.
Setting both limits of the Timeout ensure that this
Math.min(random * minTimeout * Math.pow(factor, attempt), maxTimeout) (see node-retry) will always be eq to opts.waitInterval
given that:
random=1 // i.e. no random
factor=2 // default value
attempt>=0 //
=> Math.pow(factor,attempt)>=1
=> lower bound of the Math.min>=opts.waitInterval.
=> Math.min === opts.waitInterval
| private waitUntilDoneRetryFunction( | ||
| operationToWaitFor: ResourceSnapshotsReportType | null, | ||
| onRetryCb: (report: ResourceSnapshotsReportModel) => void | ||
| ): () => Promise<void> { | ||
| return (async () => { | ||
| await this.refreshSnapshotData(); | ||
|
|
||
| const isExpectedOperation = | ||
| !operationToWaitFor || this.latestReport.type === operationToWaitFor; | ||
| const isExpectedOperation = | ||
| !operationToWaitFor || this.latestReport.type === operationToWaitFor; | ||
|
|
||
| const isOngoing = Snapshot.ongoingReportStatuses.includes( | ||
| this.latestReport.status | ||
| ); | ||
| const isOngoing = Snapshot.ongoingReportStatuses.includes( | ||
| this.latestReport.status | ||
| ); | ||
|
|
||
| iteratee(this.latestReport); | ||
| onRetryCb(this.latestReport); | ||
|
|
||
| if (!isOngoing && isExpectedOperation) { | ||
| clearTimeout(timeout); | ||
| clearInterval(interval); | ||
| resolve(); | ||
| } | ||
| }, toMilliseconds(opts.waitInterval)); | ||
| }); | ||
| if (isOngoing || !isExpectedOperation) { | ||
| throw new SnapshotOperationTimeoutError(this); | ||
| } | ||
| return; | ||
| }).bind(this); |
There was a problem hiding this comment.
extracted the function in a factory to keep things flat and simple tor ead
| } | ||
| }, toMilliseconds(opts.waitInterval)); | ||
| }); | ||
| if (isOngoing || !isExpectedOperation) { |
There was a problem hiding this comment.
Reversed the condition using De Morgan Law to keep it simple. No return needed in the end
| reject(err); | ||
| }, toMilliseconds(opts.wait)); | ||
|
|
||
| return retry( |
There was a problem hiding this comment.
We can return the retry straight away and thus remove the async from waitUntilDone and just use the promise created by async-retry
| return retry( | ||
| this.waitUntilDoneRetryFunction(operationToWaitFor, onRetryCb), | ||
| { | ||
| retries: Infinity, |
There was a problem hiding this comment.
Are you sure it supports Infinity? And that you don't have to use Number.MAX_SAFE_INTEGER
| if (isOngoing || !isExpectedOperation) { | ||
| throw new SnapshotOperationTimeoutError(this); | ||
| } | ||
| }).bind(this); |
) * remove exponential-backoff https://coveord.atlassian.net/browse/CDX-506 * add a wait flag https://coveord.atlassian.net/browse/CDX-506 * refactor snapshot https://coveord.atlassian.net/browse/CDX-506 * update test https://coveord.atlassian.net/browse/CDX-506 * lock https://coveord.atlassian.net/browse/CDX-506 * polishing https://coveord.atlassian.net/browse/CDX-506 * fix no timeout https://coveord.atlassian.net/browse/CDX-506 * lock https://coveord.atlassian.net/browse/CDX-506 * fix: use async-retry to not handle/redo the poll-timing logic (#398) * lock https://coveord.atlassian.net/browse/CDX-506 * fix merge conflict https://coveord.atlassian.net/browse/CDX-506 * fix: `org:config:monitor` doesn't show any feedback (#404) https://coveord.atlassian.net/browse/CDX-526 * refactor waitUntilDone https://coveord.atlassian.net/browse/CDX-506 * lock https://coveord.atlassian.net/browse/CDX-506 * apply corrections https://coveord.atlassian.net/browse/CDX-506 * apply corrections https://coveord.atlassian.net/browse/CDX-506 Co-authored-by: Louis Bompart <lbompart@coveo.com>
imho, this is a solved problem in the field, and we'd reinvent the wheel by not using a package to do it (it would be kinda an anti-pattern of how NPM works too)