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

Improve back-off client #1415

Merged
merged 2 commits into from
Feb 27, 2024
Merged

Conversation

lavigne958
Copy link
Collaborator

Catch usage limit errors from the Drive API. it returns the HTTP code 403 (access denied) on usage limits and access denied. Find the difference in both case and retry when it's usage limits exceeded.

Improve back-off client code.

Add pretty print for the APIerror Exception.

closes #1268
closes #1326

Catch usage limit errors from the Drive API. it returns
the HTTP code 403 (access denied) on usage limits and access denied.
Find the difference in both case and retry when it's usage limits
exceeded.

Improve back-off client code.

Add pretty print for the APIerror Exception.

closes #1268
closes #1326

Signed-off-by: Alexandre Lavigne <lavigne958@gmail.com>
@lavigne958 lavigne958 self-assigned this Feb 16, 2024
@lavigne958
Copy link
Collaborator Author

Trying to reach the API limit on the Drive API was such a pain 💥 I had to run 600 requests in parallel in order to reach the limit of 12000 requests per 60 seconds....

I believe not many users will have to backoff from this API 🙄

@alifeee
Copy link
Collaborator

alifeee commented Feb 16, 2024

these changes look good to me. two things:

Enable the test that tries to open a spreadsheet that is not shared with
the service account and receives a 403 access denied response.

Note: we must now record 403 responses with the VCR recorder.

Signed-off-by: Alexandre Lavigne <lavigne958@gmail.com>
@lavigne958
Copy link
Collaborator Author

these changes look good to me. two things:

I enabled the previously skipped test.

I am not sure how to test this new code, we have a test that tries to open a forbidden spreadsheet.
We can't make a test that reaches the limits of the quotas by sending many requests: as this will take forever to record the first time and everytime we update the cassettes. last time I ran this code I could not reach the limit of quota due to the backoff client 😵‍💫

@alifeee
Copy link
Collaborator

alifeee commented Feb 17, 2024

Can we mock the requests object for just this one test? Instead of using cassettes, we could mock requests to return 419 (or the right HTTP #), and then test that the client works as expected (likely by setting the timeout very low so it only retries once or twice)

@lavigne958
Copy link
Collaborator Author

Can we mock the requests object for just this one test? Instead of using cassettes, we could mock requests to return 419 (or the right HTTP #), and then test that the client works as expected (likely by setting the timeout very low so it only retries once or twice)

I ran some tests and I can't figure it out, I can change requests before they are made but they still need to be made, I can filter/change responses before they are returned to us but requests still need to be made with a valid response.

I can't find a way to filter that request specifically and change it's return code, in a some easy/simple ways.

it looks like a lot of work for a automatic test.

is it ok for you if we keep it like this for now ?

Copy link
Collaborator

@alifeee alifeee left a comment

Choose a reason for hiding this comment

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

is it ok for you if we keep it like this for now ?

yes :) I think people will probably not hit the rate limit enough that it is that important...

@lavigne958
Copy link
Collaborator Author

is it ok for you if we keep it like this for now ?

yes :) I think people will probably not hit the rate limit enough that it is that important...

thank you, I agree, I tried it myself... 😆 never reached it

@lavigne958 lavigne958 merged commit 4297499 into master Feb 27, 2024
10 checks passed
@lavigne958 lavigne958 deleted the feature/early_abort_on_access_denied branch February 27, 2024 20:01
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

Successfully merging this pull request may close these issues.

May be able to exclude 403 Forbidden from Backoff HTTP codes Fix BackOffHTTPClient not returning error 403
2 participants