change checksum of gcp download. add retry. #117
Conversation
context: codecov/engineering-team#1029 As suggested in the ticket we want to explore if switching the checksum validation to 'crc32c' (from 'md5') reduces the occurance of `DataCorruption` errors. I switched the function to `download_as_bytes` (see [docs](https://cloud.google.com/python/docs/reference/storage/latest/google.cloud.storage.blob.Blob#google_cloud_storage_blob_Blob_download_as_bytes)) because the existing `download_as_string` is deprecated and doesn't provide the `cheksum` argument. I also added a retry mechanism to retry the download once in case of `DataCorruption`. This is based on vibes and feelings mostly, but my line of thought is that this _might_ be a temporary issue around some part of the system downloading a file and another part updating it kinda concurrently. Also there are legitimate resons that packets are lost through the network and everything. If it is a temporary issue a secondary attempt is fairly inexpensive, and should yield good results. Plus we can see if this theory works by tracking the number of issues with the `DataCorruption` error over time compared to the number of logs for "we are retrying this". If the number of logs is low we know the change to 'crc32c' was efficient. If the number of logs is high but the number of exceptions is low we know that retrying is effective.
Codecov Report
@@ Coverage Diff @@
## main #117 +/- ##
=======================================
Coverage 89.51% 89.52%
=======================================
Files 115 115
Lines 9387 9392 +5
Branches 1497 1499 +2
=======================================
+ Hits 8403 8408 +5
Misses 781 781
Partials 203 203
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
=======================================
Coverage 89.51% 89.52%
=======================================
Files 115 115
Lines 9387 9392 +5
Branches 1497 1499 +2
=======================================
+ Hits 8403 8408 +5
Misses 781 781
Partials 203 203
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
=======================================
Coverage 91.53% 91.54%
=======================================
Files 122 122
Lines 9408 9413 +5
Branches 1611 1613 +2
=======================================
+ Hits 8612 8617 +5
Misses 781 781
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -134,11 +134,16 @@ def read_file(self, bucket_name, path, file_obj=None): | |||
| blob.content_encoding = "gzip" | |||
| blob.patch() | |||
| if file_obj is None: | |||
| return blob.download_as_string() | |||
| return blob.download_as_bytes(checksum="crc32c") | |||
There was a problem hiding this comment.
i checked the docs, apparently both download_as_string() and download_as_bytes() return bytes. i thought download_as_string() might return str which would make this potentially blow up. but it's fine
There was a problem hiding this comment.
As mentioned in the commit message the download_as_string method is deprecated. It's just an alias for download_as_bytes.
Thanks for doulbe checking it for us :)
I hope so. If you look at the docs for the upload function you'll notice we actually don't use any checksum when uploading, so it should be fine. (because the default checksum is But I'll test in staging before merging juuuust in case :P |
context: codecov/engineering-team#1029
As suggested in the ticket we want to explore if switching the checksum
validation to 'crc32c' (from 'md5') reduces the occurance of
DataCorruptionerrors.I switched the function to
download_as_bytes(see docs) because the existingdownload_as_stringisdeprecated and doesn't provide the
cheksumargument.I also added a retry mechanism to retry the download once in case
of
DataCorruption. This is based on vibes and feelings mostly,but my line of thought is that this might be a temporary issue
around some part of the system downloading a file and another part
updating it kinda concurrently. Also there are legitimate resons
that packets are lost through the network and everything.
If it is a temporary issue a secondary attempt is fairly inexpensive,
and should yield good results.
Plus we can see if this theory works by tracking the number of
issues with the
DataCorruptionerror over time compared to thenumber of logs for "we are retrying this".
If the number of logs is low we know the change to 'crc32c' was efficient.
If the number of logs is high but the number of exceptions is low
we know that retrying is effective.
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.