[gcloud] Add a setting for connect/read timeouts #1120
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mlazowik
force-pushed
the
gcloud-timeout
branch
from
0772017
to
4ee9405
Compare
sww314
reviewed
May 3, 2022
| @@ -34,7 +34,7 @@ def __init__(self, name, mode, storage): | |||
| self.mime_type = mimetypes.guess_type(name)[0] | |||
| self._mode = mode | |||
| self._storage = storage | |||
| self.blob = storage.bucket.get_blob(name) | |||
| self.blob = storage.bucket.get_blob(name, timeout=storage.timeout) | |||
There was a problem hiding this comment.
nit: you use time=storage.timeout here, but elsewhere in same function you use timeout=self._storage.timeout
I realize they are the same, but better to be consistent.
| Sometimes requests can get stuck, so it's better if the timeout is low, couple of seconds. This means that a new request | ||
| (via retry) will be made sooner. The default is higher to keep the behavior from before this setting was introduced. | ||
|
|
||
| Timeouts will be automatically retried when using `google-cloud-storage` version that includes |
There was a problem hiding this comment.
google-cloud-storage > 2.2.0
| bytes sent from the server. If float is given it's applied to both, if a tuple – the first value is for the connect | ||
| timeout, second for read. | ||
|
|
||
| Note that read timeout =/= download timeout. It’s the number of seconds that the client will wait *between* bytes sent |
There was a problem hiding this comment.
Did you mean ≠ ?
I was not sure what =/= meant.
| @@ -38,9 +38,9 @@ def test_open_read(self): | |||
|
|
|||
| f = self.storage.open(self.filename) | |||
| self.storage._client.bucket.assert_called_with(self.bucket_name) | |||
| self.storage._bucket.get_blob.assert_called_with(self.filename) | |||
There was a problem hiding this comment.
maybe define the DEFAULT_TIMEOUT=60 and use it instead of the 60 in the test. It will make the tests easier to update if the default is ever changed. It also make it very explicit what is going on.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The default that is set by the google lib is 60 seconds, which is unnecessarily high. See docs in this change for explanation why.