-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fetch with retry for mergeUI #11065
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
base: master
Are you sure you want to change the base?
fetch with retry for mergeUI #11065
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.
Nice this looks like a good approach! A few fixes/comments.
// This block catches network errors (e.g., DNS, connection refused) and the server errors we threw above. | ||
} | ||
const backoff = Math.pow(2, attempt) * initialDelay; | ||
const jitter = Math.random() * backoff; |
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.
Would it make more sense to have the jitter be based only on the initialDelay
without the exponential? That should avoid it having a range up to 2x the backoff.
// TODO: this line is just for testing locally, remove before merging | ||
CONFIGS.OL_BASE_BOOKS = 'https://openlibrary.org' | ||
|
||
export async function fetchWithRetry(resource, options = {}, maxRetries = 5, initialDelay = 5000) { |
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.
Can you experiment with 1500 or 2000 as the default? Does that work?
export async function fetchWithRetry(resource, options = {}, maxRetries = 5, initialDelay = 5000) { | |
export async function fetchWithRetry(resource, options = {}, maxRetries = 5, initialDelay = 1500) { |
@cdrini this one has incorporated all your feedback and is ready for another review. Seems to work quite well with the little check on the last 429 seen |
Closes #10875
Just a draft at the moment but it works very well.
Testing with this url
http://localhost:8080/works/merge?ol_base=openlibrary.org&records=OL54120W,OL53924W,OL8193488W,OL6037022W,OL73243W,OL192489W,OL313321W,OL15298516W,OL151880W,OL61982W,OL15150515W,OL15673999W,OL16086453W,OL15706726W,OL20600W
Technical
Testing
Screenshot
Stakeholders