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: add timeout parameter to Batch interface to match google-cloud-core #10010

Merged
merged 5 commits into from
Dec 26, 2019

Conversation

crwilcox
Copy link
Contributor

Add timeout to google cloud storage batch interface and address breakage caused by interface incompatibility.

Fixes #10009
Related to #9915

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 23, 2019
@crwilcox crwilcox changed the title feat: Add timeout parameter to Batch interface to match google-cloud-core feat: add timeout parameter to Batch interface to match google-cloud-core Dec 23, 2019
Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

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

It looks like CI is failing because of coverage dropping below 100%

coverage report --show-missing --fail-under=100
Name                                   Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------------------
google/cloud/storage/__init__.py           8      0      0      0   100%
google/cloud/storage/_helpers.py          74      0     12      0   100%
google/cloud/storage/_http.py             15      0      2      0   100%
google/cloud/storage/_signing.py         145      0     70      0   100%
google/cloud/storage/acl.py              171      0     40      0   100%
google/cloud/storage/batch.py            129      2     28      2    97%   241->242, 242, 324->325, 325
google/cloud/storage/blob.py             450      0    140      0   100%
google/cloud/storage/bucket.py           530      0    168      0   100%
google/cloud/storage/client.py           163      0     56      0   100%
google/cloud/storage/constants.py         11      0      0      0   100%
google/cloud/storage/hmac_key.py          81      0     26      0   100%
google/cloud/storage/iam.py               23      0      0      0   100%
google/cloud/storage/notification.py     116      0     32      0   100%
tests/unit/__init__.py                     0      0      0      0   100%
tests/unit/test__helpers.py              238      0      0      0   100%
tests/unit/test__http.py                  44      0      0      0   100%
tests/unit/test__signing.py              409      0     52      0   100%
tests/unit/test_acl.py                   669      0      2      0   100%
tests/unit/test_batch.py                 386      0      6      0   100%
tests/unit/test_blob.py                 2156      0    110      0   100%
tests/unit/test_bucket.py               2062      0     46      0   100%
tests/unit/test_client.py                679      0     10      0   100%
tests/unit/test_hmac_key.py              289      0      6      0   100%
tests/unit/test_notification.py          304      0      2      0   100%
----------------------------------------------------------------------------------
TOTAL                                   9152      2    808      2    99%
Command coverage report --show-missing --fail-under=100 failed with exit code 2
Session cover failed.

subrequest = MIMEApplicationHTTP(method, uri, headers, body)
multi.attach(subrequest)
timeout = timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Could one of the timeouts be renamed? I had a little trouble figuring out what was happening here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, lord. My bad. Let me fix that. And my bad also, didn't run coverage locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related: I am defaulting to the timeout of the last request added to the batch. I could just as well see adding the timeouts up for each request in batch. Either seems like a fair enough approach.

Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

Thanks!

Re: taking the last vs adding them up. I have a slight preference for the current behavior of taking the last timeout value. Since the point of a batch request is to make only one HTTP request, I think it's more intuitive to take a single value rather than add them up.

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

Successfully merging this pull request may close these issues.

Update Storage after #9915, incompatible change.
5 participants