-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat: Customize retry implementation #680
Conversation
* feat: began retry configuration implementation * 🦉 Updates from OwlBot * added semicolon * 🦉 Updates from OwlBot * removed log statement * refactored constants * updated retry-request and made relevant updates in this lib * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
* feat: implemented retry function * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * refactored retryableErrorFn into retry config * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * refactored code so test stub works as expected * removed print * added test * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * refactored function Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
return err && util.shouldRetryRequest(err); | ||
}, | ||
maxRetryDelay: config.retryOptions?.maxRetryDelay, | ||
retryDelayMultiplier: config.retryOptions?.retryDelayMultiplier, | ||
totalTimeout: config.retryOptions?.totalTimeout, |
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.
@stephenplusplus The system test is failing because it is passing retry-request
undefined for these three values (maxRetryDelay
, retryDelayMultiplier
, and totalTimeout
). Do you think that common should be defining defaults and always passing those defaults here? Or should retry-request
be using its defaults if it receives an undefined
value?
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.
retry-request
! PTAL: googleapis/retry-request#37
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.
Amazing! Thank you :)
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.
Thanks for catching it! The fix is now released in 4.2.2.
smh at myself. Sorry to auto-close this 😞 |
Developed for supporting retries in nodejs-storage. https://github.com/googleapis/nodejs-storage/pull/1493/files
Unit tests pass. Have tested locally linked with storage and that works as expected. Conformance testing will be written from storage to verify retry functionality further.
Adds the
RetryOptions
interface which allows users to configureretryDelayMultiplier
,totalTimeout
,maxRetryDelay
,autoRetry
,maxRetries
, andretryableErrorFn