-
Notifications
You must be signed in to change notification settings - Fork 309
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: apply timeout to all resumable upload requests #1070
Conversation
Test failure:
I think we'll have to bump the minimum version of the resumable media library. |
@tswast The feature was added (by @plamut) in June 2020, and released with version 0.6.0 of The test failure comes from the broken stub version in the testcase. |
may be repeated several times using the same timeout each time. | ||
|
||
Can also be passed as a tuple (connect_timeout, read_timeout). | ||
See :meth:`requests.Session.request` documentation for details. |
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.
I'm curious, does the same apply to our other API requests? I know we use requests
library for those too, but via google-cloud-core
.
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.
From what I can see, it does. The timeout
argument gets passed down to google.cloud._http.JSONConnection
, which accepts a two-tuple.
I think we did not advertise this second option in BigQuery and just went with Optional[Union[int, float]]
to avoid leaking transport implementation details, but I'm not 100%.
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.
google-cloud-core
definitely exposes both float
and tuple
for timeout
.
Note that its current docs link (under cloud.google.com
) doesn't show those details.
* fix: apply timeout to all resumable upload requests * Fix stub in test case * Improve timeout type and other type annotations * Annnotate return type of _do_resumable_upload()
Fixes #1067.
PR checklist: