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

fix: retry uploads only conditionally #316

Merged
merged 3 commits into from Nov 20, 2020
Merged

fix: retry uploads only conditionally #316

merged 3 commits into from Nov 20, 2020

Conversation

@andrewsg
Copy link
Contributor

@andrewsg andrewsg commented Nov 16, 2020

No description provided.

@google-cla google-cla bot added the cla: yes label Nov 16, 2020
@andrewsg andrewsg changed the title Retry uploads only conditionally fix: retry uploads only conditionally Nov 16, 2020
@andrewsg andrewsg force-pushed the retry-resumable branch from 5a8763f to c535039 Nov 16, 2020
@andrewsg andrewsg force-pushed the retry-resumable branch from c535039 to ccf37a1 Nov 16, 2020
@@ -2058,6 +2060,15 @@ def _do_upload(
**only** response in the multipart case and it will be the
**final** response in the resumable case.
"""
if if_metageneration_match is None and num_retries is None:
Copy link
Contributor

@tritone tritone Nov 16, 2020

I think we should update the docstring as well to make it clear that setting num_retries can be used to override the idempotency requirement (assuming that's the intention), to make sure that users know the implications.

Copy link
Contributor Author

@andrewsg andrewsg Nov 16, 2020

Yes, that's the intention. Will do.


# Adjust num_retries expectations to reflect the conditional default in
# _do_upload()
if num_retries is None and if_metageneration_match is None:
Copy link
Contributor

@tritone tritone Nov 16, 2020

Can you comment on to what extent the conditional is covered by test cases?

Copy link
Contributor

@tseaver tseaver Nov 16, 2020

@tritone the cover session under nox requires (since the merge of #308) 100% coverage (branch and statements).

Copy link
Contributor

@tritone tritone Nov 16, 2020

Ah yeah that's right, good point.

Copy link
Contributor Author

@andrewsg andrewsg Nov 16, 2020

Branch coverage of the code I've added is 100% according to the coverage tool. We are exercising scenarios where if_metageneration_match = true and num_retries is not None.

@gcf-merge-on-green
Copy link
Contributor

@gcf-merge-on-green gcf-merge-on-green bot commented Nov 20, 2020

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, or one of your required reviews was not approved. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@andrewsg andrewsg merged commit 547740c into master Nov 20, 2020
5 checks passed
@tseaver tseaver deleted the retry-resumable branch Nov 24, 2020
shaffeeullah added a commit to shaffeeullah/python-storage that referenced this issue Jan 26, 2021
* fix: retry uploads only conditionally

* update docstring for num_retries
shaffeeullah added a commit to shaffeeullah/python-storage that referenced this issue Jan 26, 2021
* fix: retry uploads only conditionally

* update docstring for num_retries
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
* fix: retry uploads only conditionally

* update docstring for num_retries
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
* fix: retry uploads only conditionally

* update docstring for num_retries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants