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.

Sign up for GitHub

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

Jump to bottom

fix: replace default retry for upload operations #480

Merged
merged 11 commits into from
Jun 30, 2021
Merged

fix: replace default retry for upload operations #480

merged 11 commits into from
Jun 30, 2021

Conversation

cojenco
Copy link
Contributor

@cojenco cojenco commented Jun 25, 2021 •
edited
Loading

This PR replaces upload operations default retries with storage.retry.DEFAULT_RETRY_IF_GENERATION_SPECIFIED and aligns with the retry strategy conditional idempotency.

With default retries set to storage.retry.DEFAULT_RETRY_IF_GENERATION_SPECIFIED, passing in ifGenerationMatch makes the upload operation (1) conditional and (2) only retried by default on whether the object's current generation matches the given value. Setting if_generation_match=0 makes the upload operation succeed only if there are no live versions of the object (fresh uploads) and supports retries.

Fixes #477 🦕

@cojenco cojenco requested a review from a team June 25, 2021 16:44
@cojenco cojenco requested a review from a team as a code owner June 25, 2021 16:44
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 25, 2021
@cojenco cojenco mentioned this pull request Jun 25, 2021
Merged
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label Jun 26, 2021
andrewsg
andrewsg approved these changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@andrewsg andrewsg left a comment

Choose a reason for hiding this comment

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

Good work here. Please amend the PR title to include that create_resumable_upload_session is being amended in the same PR (or split it off into a separate PR) so that the change is included in the release notes.

google/cloud/storage/blob.py
@@ -1750,7 +1749,7 @@ def _do_multipart_upload(

This private method does not accept ConditionalRetryPolicy values
because the information necessary to evaluate the policy is instead
evaluated in client.download_blob_to_file().
evaluated in blob._do_upload().
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it looks like we're evaluating it BOTH in blob._do_upload() and in client.download_blob_to_file(). This is harmless, which is why the tests didn't catch it, but we only need one of those.

I don't have a strong opinion on whether we do this in _do_upload or download_blob_to_file. Let's keep this docstring change and additionally we can remove the conditional retry policy code in client.download_blob_to_file, either in another PR, or in this one if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, I believe we'll need to remain both ConditionalRetryPolicy evaluations as one is for upload operations and the other is for downloads. I revised the docstrings here to match particularly where we're doing the evaluations for upload operations, blob._do_upload() .

tritone
tritone reviewed Jun 29, 2021
View reviewed changes
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Nice catch on this! A couple questions

google/cloud/storage/blob.py Show resolved Hide resolved
google/cloud/storage/blob.py Outdated Show resolved Hide resolved
tests/unit/test_blob.py Outdated Show resolved Hide resolved
@cojenco cojenco mentioned this pull request Jun 29, 2021
Merged
@cojenco cojenco mentioned this pull request Jun 30, 2021
Merged
4 tasks
@cojenco cojenco merged commit c027ccf into master Jun 30, 2021
@cojenco cojenco deleted the update-upload-retry-477 branch June 30, 2021 16:23
cojenco added a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
@cojenco
fix: replace default retry for upload operations (googleapis#480)
d373d9f 
* fix: revise upload operations preconditions to if_generation_match

* fix docstrings

* add retry configuration to blob.create_resumable_upload_session

* align var values in test

* test coverage

* Revert "test coverage"

This reverts commit e91916f.

* Revert "align var values in test"

This reverts commit aec585b.

* Revert "add retry configuration to blob.create_resumable_upload_session"

This reverts commit 8c1ae3c.

* revise tests after reverting
cojenco added a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
@cojenco
fix: replace default retry for upload operations (googleapis#480)
9330b3f 
* fix: revise upload operations preconditions to if_generation_match

* fix docstrings

* add retry configuration to blob.create_resumable_upload_session

* align var values in test

* test coverage

* Revert "test coverage"

This reverts commit e91916f.

* Revert "align var values in test"

This reverts commit aec585b.

* Revert "add retry configuration to blob.create_resumable_upload_session"

This reverts commit 8c1ae3c.

* revise tests after reverting
@release-please release-please bot mentioned this pull request Jan 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewsg andrewsg andrewsg approved these changes

@tritone tritone tritone approved these changes

@tseaver tseaver Awaiting requested review from tseaver

Assignees
No one assigned
Labels
api: storage Issues related to the googleapis/python-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Uploads and if_metageneration_match and retries
3 participants
@cojenco @andrewsg @tritone