Skip to content
This repository was archived by the owner on Mar 31, 2026. It is now read-only.

feat: add configurable checksumming for blob uploads and downloads - #246

Merged
tseaver merged 14 commits into
masterfrom
crc32c
Aug 26, 2020
Merged

feat: add configurable checksumming for blob uploads and downloads#246
tseaver merged 14 commits into
masterfrom
crc32c

Conversation

@andrewsg

@andrewsgandrewsg commented Aug 14, 2020

Copy link
Copy Markdown
Contributor

No description provided.

@google-clagoogle-claBot added the cla: yes This human has signed the Contributor License Agreement. label Aug 14, 2020
andrewsg
andrewsg commented Aug 14, 2020
View reviewed changes
Comment threadgoogle/cloud/storage/blob.py
response = upload.transmit_next_chunk(transport, timeout=timeout)
except resumable_media.DataCorruption:
# Attempt to delete the corrupted object.
self.delete()

@andrewsgandrewsgAug 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

If this fails for any reason, both exceptions will be reported, using the Python exception pattern "During handling of the above exception, another exception occurred"

@tseavertseaverAug 17, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What occurs here if a valid previous generation of the blob exists?

@andrewsgandrewsgAug 21, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

In the default case, if versioning is off, there is no way to recover the older version. Unfortunately this can only be resolved by backend changes to make server-side checksumming possible for resumable uploads.

In the case where versioning is on, the previous generation still exists, but is not live. This is consistent with behavior in nodejs, which I modeled this code after. We could potentially try to roll back and make the previous version live, but we should also modify nodejs and other languages to be consistent across languages. What do you think?

@frankynfrankynAug 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is more of a documentation issue until there's a better fix in the backend to support checksums better.

I'd prefer consistency here with clear acknowledgment that these cases can occur when using checksumming. It's not done automatically so it doesn't change behavior for uploads.

andrewsg
andrewsg commented Aug 14, 2020
View reviewed changes
Comment threadgoogle/cloud/storage/blob.py
:type checksum: str
:param checksum:
(Optional) The type of checksum to compute to verify
the integrity of the object. If the upload is completed in a single

@andrewsgandrewsgAug 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This "if the upload is completed in a single request" / "if the upload is too large" is undeniably ugly and hopefully we can implement server-side handling of resumable upload checksums to resolve that soon

@andrewsgandrewsg changed the title Add configurable checksumming for blob uploads and downloadsfeat: add configurable checksumming for blob uploads and downloadsAug 14, 2020
@andrewsg

andrewsg commented Aug 14, 2020

Copy link
Copy Markdown
ContributorAuthor

Recommend hold off on merging until google-resumable-media releases to 1.0 (and bump up the dependency version number in setup.py here)

tseaver
tseaver suggested changes Aug 17, 2020
View reviewed changes
Comment threadtests/system/test_system.py Outdated
@product-auto-labelproduct-auto-labelBot added the api: storage Issues related to the googleapis/python-storage API. label Aug 21, 2020
frankyn
frankyn suggested changes Aug 21, 2020
View reviewed changes
Comment threadtests/system/test_system.py
Comment threadgoogle/cloud/storage/blob.py
response = upload.transmit_next_chunk(transport, timeout=timeout)
except resumable_media.DataCorruption:
# Attempt to delete the corrupted object.
self.delete()

@frankynfrankynAug 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is more of a documentation issue until there's a better fix in the backend to support checksums better.

I'd prefer consistency here with clear acknowledgment that these cases can occur when using checksumming. It's not done automatically so it doesn't change behavior for uploads.

@tseaver

tseaver commented Aug 24, 2020

Copy link
Copy Markdown
Contributor

googleapis/google-resumable-media-python#142 is the PR for bumping GRMP to 1.0.

@andrewsg

andrewsg commented Aug 24, 2020

Copy link
Copy Markdown
ContributorAuthor

@tseaver@frankyn PTAL

@tseaver Has the PR you linked made it to release yet? If it's released I'll use this PR to also update setup.py to use the new version.

frankyn
frankyn reviewed Aug 25, 2020
View reviewed changes
Comment threadgoogle/cloud/storage/blob.py
@andrewsg

andrewsg commented Aug 25, 2020

Copy link
Copy Markdown
ContributorAuthor

Bumped version number. This should be good to merge as soon as final review comes through. Thanks all

frankyn
frankyn approved these changes Aug 25, 2020
View reviewed changes

@frankynfrankyn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @andrewsg!

@andrewsg

andrewsg commented Aug 26, 2020

Copy link
Copy Markdown
ContributorAuthor

@tseaver PTAL soon if you are available, I've been asked to release. Thanks!

@tseaver

tseaver commented Aug 26, 2020

Copy link
Copy Markdown
Contributor

https://pypi.org/project/google-resumable-media/1.0.0/ is released.

@tseaver
tseaver merged commit 23b7d1c into masterAug 26, 2020
@tseaver
tseaver deleted the crc32c branch August 26, 2020 19:20
@tseavertseaver mentioned this pull request Dec 1, 2020
Closed
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
@andrewsg@tseaver
@frankyn@cojenco
feat: add configurable checksumming for blob uploads and downloads (g…
c149785
…oogleapis#246)
Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
@andrewsg@tseaver
@frankyn@cojenco
feat: add configurable checksumming for blob uploads and downloads (g…
ed18479
…oogleapis#246)
Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
@release-pleaserelease-pleaseBot mentioned this pull request Jan 12, 2022
Closed
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crwilcoxcrwilcoxAwaiting requested review from crwilcox
2 more reviewers
@tseavertseavertseaver approved these changes
@frankynfrankynfrankyn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: storageIssues related to the googleapis/python-storage API.cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@andrewsg@tseaver@frankyn