Skip to content

Conversation

cojenco
Copy link
Contributor

@cojenco cojenco commented Jun 25, 2021

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 requested review from tritone, tseaver and andrewsg June 25, 2021 20:59
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label Jun 26, 2021
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.

@@ -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() .

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

@cojenco cojenco mentioned this pull request Jun 30, 2021
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
* 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
* 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
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 googleapis/python-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.

Uploads and if_metageneration_match and retries
3 participants