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

feat(resumable-media): add customizable timeouts to upload/download methods #116

Merged
merged 10 commits into from Jun 29, 2020

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Jan 8, 2020

Fixes #45.

This PR adds configurable timeouts to upload objects.

Motivation:
We would like to add timeouts to BigQuery methods that depend on the file upload functionality (using resumable media), so that requests do not get stuck indefinitely at the transport layer.

Currently there is a default timeout used (61, 60), but that might not suit all use cases, hence the need for optional explicit timeout parameters.

CAUTION: Changing public function signatures might break some libraries that utilize resumable media, even though the new arguments are optional, and positioned last), and this will not be caught by the test suite.

It has happened recently when we added timeouts to google-auth, thus it is advisable that code owners of the dependent libraries have a look, too (if that is feasible).

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 8, 2020
@plamut plamut changed the title feat(resumable-media): Add customizable timeouts to uploads. feat(resumable-media): add customizable timeouts to uploads Jan 8, 2020
@plamut
Copy link
Contributor Author

plamut commented Jan 8, 2020

Unit tests suite fails under Python 3.4, created an issue (#117). The 3.4 tests pass, though, if ran in isolation.

Update: Will beWas addressed in #118.

@plamut plamut marked this pull request as ready for review January 8, 2020 16:30
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2020
@anantzoid
Copy link

anantzoid commented Jun 15, 2020

I've been facing ReadTimeout issues while reading the data from GCS and noticed _DEFAULT_READ_TIMEOUT = 60 cannot be changed by the user for read requests [src].

The user should be able to specify timeout while using download_to_file and similar functions.

@plamut plamut requested a review from busunkim96 June 15, 2020 11:36
@plamut
Copy link
Contributor Author

plamut commented Jun 15, 2020

@busunkim96 Somehow this slipped out of the focus, and it might be a good time to revive it. Added you as a reviewer based on the GitHub's suggestion, but feel free to delegate to someone else if that makes more sense, thanks!

@anantzoid Thanks for bringing this back to attention. As this PR only adds configurable timeouts to uploads, I can add the same for the download methods, too, provided that the original feature request is confirmed (I personally don't see any reason why it shouldn't be).

@busunkim96 busunkim96 requested a review from crwilcox June 15, 2020 17:20
@busunkim96
Copy link
Contributor

LGTM'd, but adding @crwilcox since the storage folks are other primary users of this library

@plamut plamut added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 16, 2020
@plamut
Copy link
Contributor Author

plamut commented Jun 16, 2020

I will update the PR with timeouts to the download methods as well.

@plamut plamut changed the title feat(resumable-media): add customizable timeouts to uploads feat(resumable-media): add customizable timeouts to upload/download methods Jun 16, 2020
@plamut plamut removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 17, 2020
@plamut
Copy link
Contributor Author

plamut commented Jun 23, 2020

@crwilcox Friendly ping, can you take a look? Thanks!

@plamut plamut merged commit 5310921 into googleapis:master Jun 29, 2020
@plamut plamut deleted the bq-10005 branch June 29, 2020 06:48
@plamut
Copy link
Contributor Author

plamut commented Jun 29, 2020

@busunkim96 @frankyn Thanks for the reviews!

When would it be possible to release a new version of google-resumable-media in order to unblock googleapis/python-storage#185 ? I hear this week is a holiday period there?

@frankyn
Copy link
Member

frankyn commented Jun 29, 2020

Hi @plamut, let's wait until next week when everyone is back.

@plamut
Copy link
Contributor Author

plamut commented Jun 29, 2020

Sounds good, there's no rush.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add request timeout
6 participants