-
Notifications
You must be signed in to change notification settings - Fork 156
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: do not spam the log with checksum related INFO messages when downloading using transfer_manager #1357
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a reasonable patch to avoid passing in the default checksum md5
in the subsequent call blob._prep_and_do_download
. @andrewsg thoughts/concerns to this change?
@rafalh Could you update the corresponding unit tests in https://github.com/googleapis/python-storage/blob/main/tests/unit/test_transfer_manager.py?
This makes sense to me. Thank you for your submission! |
…nloading using transfer_manager `download_chunks_concurrently` function does not allow to set `checksum` field in `download_kwargs`. It also does not set it on its own so it takes the default value of `"md5"` (see `Blob._prep_and_do_download`). Because ranged downloads do not return checksums it results in a lot of INFO messages (tens/hundreds): ``` INFO google.resumable_media._helpers - No MD5 checksum was returned from the service while downloading ... (which happens for composite objects), so client-side content integrity checking is not being performed. ``` To fix it set the `checksum` field to `None` which means no checksum checking for individual chunks. Note that `transfer_manager` has its own checksum checking logic (enabled by `crc32c_checksum` argument)
I fixed the tests and got them to pass locally (executed only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for working on this!
download_chunks_concurrently
function does not allow to setchecksum
field indownload_kwargs
. It also does not set it on its own so it takes the default value of"md5"
(seeBlob._prep_and_do_download
). Because ranged downloads do not return checksums it results in a lot of INFO messages (tens/hundreds):To fix it set the
checksum
field toNone
which means no checksum checking for individual chunks. Note thattransfer_manager
has its own checksum checking logic (enabled bycrc32c_checksum
argument)Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #1358 🦕