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

Documenting about the two types of retry logics for 429 errors. #38

Open
sushaanttb opened this issue Feb 21, 2024 · 2 comments
Open

Documenting about the two types of retry logics for 429 errors. #38

sushaanttb opened this issue Feb 21, 2024 · 2 comments

Comments

@sushaanttb
Copy link
Contributor

sushaanttb commented Feb 21, 2024

Please see this #34

The current documentation does not mention about the presence of the two retry logics in the code (& how they are coupled currently). However, it's important for the end user to be aware about it and hence, ideally it should be documented.

@sushaanttb sushaanttb changed the title Decoupling Throttling retries logic & Throttling backoff logic. Documenting about the two types of retry logics for 429 errors. Feb 21, 2024
@michaeltremeer
Copy link
Contributor

I am copy-pasting my response from your other issue for anyone else who reads this issue:

Hey @sushaanttb,

Have a look at #21 and the discussion within. You will see some more info there about what is going on with different retry settings, and how MAX_RETRY_SECONDS effects the outputs. That PR has not been merged and it doesn't look like it will, so if you want the behaviour you suggest, I would suggest you use my fork which fixes this issue and uses a more suitable value of 60 seconds for MAX_RETRY_SECONDS. My fork has a long section in the README discussing how the two params work and when they should be used.

@sushaanttb
Copy link
Contributor Author

sushaanttb commented Feb 22, 2024

Hey @michaeltremeer

Thanks for the comment!

I did had a look on the existing issues, including Issue#20 but as it was marked closed and then as your PR was also "still" not merged in the main branch, I was a bit confused on why it was still not merged in the main repo and as it was also becoming a bit confusing. I accordingly re-raised some of these issues again.

Btw, your PR got merged yesterday after I raised those issues, so congratulations and great work!.

Lastly, I like how the documentation is now more clear but somehow I do believe on the need of talking about the two types of retry logics we have as which was what I originally meant.

For e.g.: the revised documentation now explains that if retry=none then no retries will be made at all.
However, it still doesn't talks about two types of retries(one using retry header value and one using exponential backoff).

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants