twilio
Add exponential backoff
Fixes #713
Add exponential backoff and 3 retries on 429 error per the recommendations
Checklist
- [x] I acknowledge that all my contributions will be made under the project's license
- [x] I have made a material change to the repo (functionality, testing, spelling, grammar)
- [x] I have read the Contribution Guidelines and my PR follows them
- [x] I have titled the PR appropriately
- [x] I have updated my branch with the main branch
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] I have added the necessary documentation about the functionality in the appropriate .md file
- [x] I have added inline documentation to the code I modified
If you have questions, please file a support ticket, or create a GitHub Issue in this repository.
I feel like this should be enabled by default, with an option to disable it through the ClientOpts.
Given that these retries are documented by Twilio as 'best practices' ( https://www.twilio.com/docs/usage/rest-api-best-practices#retry-with-exponential-backoff) I feel like the official API client should follow recommended best practices by default, rather than it being an option that users have to know about and explicitly turn on.
On Tue, Nov 29, 2022 at 9:54 PM childish-sambino @.***> wrote:
@.**** commented on this pull request.
Looking to get this into one of the upcoming release candidates. For this, could you switch the base branch to 4.0.0-rc and resolve the merge conflicts?
Also, I think it would be good to make this disabled by default with an option to enable it through the ClientOpts (and updating README).
Either way, I'm bumping the internal ticket up the backlog in case we're able to get to it first (ref: DI-2446).
— Reply to this email directly, view it on GitHub https://github.com/twilio/twilio-node/pull/741#pullrequestreview-1198408255, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKKYRBG7EF5634BCXYJDLWKZ3RDANCNFSM5PCARK5Q . You are receiving this because you authored the thread.Message ID: @.***>
@bobtfish Fair point. I think that's more true for new users expecting it to already be enabled, but there is greater impact for large existing customers not realizing the change when upgrading. The opt-in is safer for all, especially given that it's a new feature. Similar to lazy-loading, we'd flip to enabled by default in a future MV roll once it's been around a while with good feedback (which we're doing for lazy-loading in v4 RC).
