Skip to content
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

fix: bump minimal retry in case of secondary rate limiting to 60s #594

Merged
merged 11 commits into from
May 19, 2023

Conversation

gr2m
Copy link
Contributor

@gr2m gr2m commented May 15, 2023

I had a GitHub-internal discussion about this. We cannot give details on how secondary limit rating works as it is meant to avoid API abuse. But for now we came to an agreement that waiting 60s for retries is a good default implementation for handling.

Closes #454, closes #566.

@gr2m gr2m force-pushed the bump-minimal-retry-for-secondary-rate-limit-to-60s branch from 0994735 to eccc64a Compare May 15, 2023 18:49
kfcampbell
kfcampbell previously approved these changes May 15, 2023
Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing tests are unrelated and there's a PR in flight to address them. I'll approve now to avoid unblocking this from shipping once that PR is in.

@gr2m
Copy link
Contributor Author

gr2m commented May 16, 2023

Sorry I wasn't quite done. The failing tests were legit. I forgot that we have to properly deprecate the minimalSecondaryRateRetryAfter option in order to avoid a breaking change. I did some refactoring that really should be a follow up PR but I don't know when I'll have time for it so I just put it in while at it. Sorry 🙇🏼 And I moved testing for deprecations into its own test file to make it easier to remove deprecations as part of the next breaking change.

@gr2m gr2m added the Type: Feature New feature or request label May 16, 2023
@gr2m gr2m added Type: Bug Something isn't working as documented and removed Type: Feature New feature or request labels May 16, 2023
…ent settings?! No idea, not worth investigating as the code gets removed soon anyway
Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh shoot! I'm bummed I mis-recognized that. Thanks for explaining!

@gr2m gr2m merged commit 5335870 into main May 19, 2023
@gr2m gr2m deleted the bump-minimal-retry-for-secondary-rate-limit-to-60s branch May 19, 2023 21:43
@github-actions
Copy link
Contributor

🎉 This PR is included in version 5.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Bug Something isn't working as documented
Projects
None yet
2 participants