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

gcloud: Use DEFAULT_RETRY when uploading a file. #1156

Merged
merged 3 commits into from Aug 10, 2022

Conversation

Hatell
Copy link
Contributor

@Hatell Hatell commented Jul 15, 2022

Use a default retry policy when uploading a file to Google Storage.

@jschneier
Copy link
Owner

What motivates this change?

@Hatell
Copy link
Contributor Author

Hatell commented Jul 18, 2022

Recently I have had issues with Google Cloud Storage. Some times I got 503 and I had few data losses. GCS documentation says that when server answer 503 you need retry and use retry policy to automate it.

The default retry policy of upload_from_file is DEFAULT_RETRY_IF_GENERATION_SPECIFIED and call of upload_from_file doesn't specify generation so there is no retry.

@jschneier
Copy link
Owner

Okay, this change looks fine if you fix CI. Will need a rebase.

@Hatell
Copy link
Contributor Author

Hatell commented Aug 8, 2022

Rebase is now done.

@jschneier
Copy link
Owner

Thanks, can you please fix the tests.

@Hatell
Copy link
Contributor Author

Hatell commented Aug 9, 2022

Now tests are fixed.

@Hatell
Copy link
Contributor Author

Hatell commented Aug 9, 2022

I fixed those flake8 issues.

@jschneier
Copy link
Owner

@Hatell what do you think about adding retry=DEFAULT_RETRY to .delete()? I think that's now the only place where there are no retries left.

For whatever reason it looks like the client lib uses DEFAULT_RETRY for all readonly operations and DEFAULT_RETRY_IF_GENERATION_SPECIFIED for all write/delete operations.

@Hatell
Copy link
Contributor Author

Hatell commented Aug 9, 2022

I think that adding retry to delete is good idea. Can you do that now? I can do it tomorrow.

@jschneier
Copy link
Owner

Not sure if I have permissions to push to your branch, will let you do it.

@Hatell
Copy link
Contributor Author

Hatell commented Aug 10, 2022

Retry is added to delete_blob call.

@jschneier jschneier merged commit f18d68e into jschneier:master Aug 10, 2022
jschneier added a commit that referenced this pull request Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants