JSUI-2153 Adding BackOff module with queuing logic #862
Conversation
samisayegh
commented
Jun 29, 2018
samisayegh
commented
src/misc/BackOff.ts
Outdated
| } | ||
| } | ||
|
|
||
| throw {}; |
src/rest/SearchEndpoint.ts
Outdated
| _.defer(function() { | ||
| return ''; | ||
| }); | ||
| const tokenWasRenewed = await this.renewAccessTokenIfExpired(error); |
There was a problem hiding this comment.
I'd expect the code to read more like a big switch case on the error code, versus trying one error handler function. and if it does not work, go to the next error handler.
Seems much more straightforward to execute all error handler only when they are actually needed.
src/rest/SearchEndpoint.ts
Outdated
| throw this.handleErrorResponse(error) as any; | ||
| } | ||
| } | ||
|
|
||
| private is429Error(error: IErrorResponse) { |
There was a problem hiding this comment.
I'd call this "isThrottled" versus the error code number.
src/rest/BackOffRequest.ts
Outdated
| private static queue: Request[] = []; | ||
| private static clearingQueue = false; | ||
|
|
||
| public static enqueue(request: IBackOffRequest): Promise<ISuccessResponse<{}>> { |
There was a problem hiding this comment.
I'd change the definition to use the generics you are "semi" using.
type Request<T> = () => Promise<ISuccessResponse<T>>;
export interface IBackOffRequest<T> {
fn: Request<T>;
retry?: (e, attemptNumber: number) => boolean;
}
enqueue<T>(request: IBackOffRequest<T>): Promise<ISuccessResponse<T>>
That kinda stuff, so you would not have to cast when you use it inside SearchEndpoint :
const backOffRequest = { fn: request, retry: this.retryIf429Error };
const response = await BackOffRequest.enqueue<T>(backOffRequest);
return response.data;
src/rest/SearchEndpoint.ts
Outdated
| return this.accessToken.isExpired(e) && this.accessToken.doRenew(); | ||
| } | ||
|
|
||
| private async backOff429Request<T>(request: () => Promise<ISuccessResponse<{}>>) { |
There was a problem hiding this comment.
Same comment about the 429 in the method name, I'd change the naming to "throttled".
src/rest/SearchEndpoint.ts
Outdated
|
|
||
| private retryIf429Error(e: IErrorResponse, attempt: number) { | ||
| if (this.is429Error(e)) { | ||
| new Logger(this).info(`Resending the request because it was throttled. Retry attempt ${attempt}`); |
There was a problem hiding this comment.
I think you should already have a Logger instantiated in the SearchEndpoint constructor which you can re-use..
src/misc/BackOff.ts
Outdated
|
|
||
| const defaultOptions: IBackOffOptions = { | ||
| numOfAttempts: 10, | ||
| timeStep: 2, |
There was a problem hiding this comment.
Isn't that a bit too low ? 🤔 Not 100% sure TBH, just wondering.
unitTests/misc/BackOffTest.ts
Outdated
| @@ -0,0 +1,31 @@ | |||
| import { backOff } from '../../src/misc/BackOff'; | |||
|
|
|||
| export function BackOffTest() { | |||
There was a problem hiding this comment.
Hum seems like we are not testing an awful lot in that utility module.
Can we check that we have sane default ? (eg: 10 number of retry, don't have to pass in a retryConditionOption, etc.).
We should also be checking that all the possible options on that module works correctly.
Eg: number of attempts, delay, retryCondition being called, etc.
unitTests/rest/BackOffRequestTest.ts
Outdated
| data: {} | ||
| }; | ||
|
|
||
| beforeAll(() => stubBackOffDependency()); |
There was a problem hiding this comment.
Shouldn't we make sure that we change the dependency back to the original module on afterEach()?
unitTests/rest/BackOffRequestTest.ts
Outdated
| BackOffRequest.enqueue({ fn: () => resolveInOneTimeUnit() }); | ||
| const secondRequest = BackOffRequest.enqueue({ fn: () => resolveInOneTimeUnit() }); | ||
|
|
||
| secondRequest.then(response => { |
There was a problem hiding this comment.
Could we have a test that does something like this ?
const firstRequest = ()=> resolveIn5TimeUnits();
const secondRequest = ()=> resolveInOneTimeUnits();
firstRequest.then(()=> {
expect second request to not be resolved
})
And then another one where you add a retryCondition on the first and second request, and you check that the retry condition on the second request is not called before the first request is finished ?
