-
-
Notifications
You must be signed in to change notification settings - Fork 209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set retry limit for LastFMImporter #1339
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. No changes required as such. Just a few observations I think can solidify the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality seems sane, though I've not tested it myself.
To confirm the expected functionality:
Before this PR, we just repeatedly retried on http 5xx error. We never retried on 4xx error.
With this change, we now only retry LASTFM_RETRIES times if there is a 5xx page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic looks good, approving for speedy deployment.
One comment about the user-facing side of things, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed a spot!
Unless I'm mistaken, whenever the retry
function is called, it should be await
ed and we should be returning its content.
For example:
retry(
Got ${response.status});
-> return await retry(
Got ${response.status});
if (retries <= 0) { | ||
return null; | ||
} | ||
await new Promise((resolve) => setTimeout(resolve, timeout)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just asking a question for my information: will this just pause execution for timeout
ms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Sort of an inline setTimeout for the async/await age.
Jest fake timers don't play well with promises and setTimeout so we replace the setTimeout function and don't have to wait for the real timeout. See jestjs/jest#7151
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silly to approve myself, but I've done some extra testing and it all seems to work, and the code is a lot neater now, plus extra tests.
The
LastFMImporter
used to retry indefinitely in case of errors (e.g. 50x). Further, when a page import failed, we used to move to the next page, while the previous pages continued to retry.We are now restricting the number of retries to 3. We also do not move to the next page while we are still retrying the current page.
A few tests have also been added to ensure that the retry logic works as expected.