Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: do not spam the log with checksum related INFO messages when downloading using transfer_manager#1357

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
cojenco merged 2 commits intogoogleapis:mainfromrafalh:patch-1
Oct 9, 2024

Conversation

@rafalh
Copy link
Contributor

@rafalhrafalh commentedOct 2, 2024
edited
Loading

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):

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 thechecksum 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:

  • Make sure to open an issue as abug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes#1358 🦕

@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcfbot commentedOct 2, 2024
edited
Loading

🤖 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 useautomerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-labelproduct-auto-labelbot added size: xsPull request size is extra small. api: storageIssues related to the googleapis/python-storage API. labelsOct 2, 2024
@rafalhrafalh changed the titleFix checksum related INFO messages spamming the log when downloading …Fix checksum related INFO messages spamming the log when downloading using transfer_managerOct 2, 2024
@rafalhrafalh changed the titleFix checksum related INFO messages spamming the log when downloading using transfer_managerfix: do not spam the log with checksum related INFO messages when downloading using transfer_managerOct 2, 2024
@rafalhrafalh marked this pull request as ready for reviewOctober 2, 2024 10:44
@rafalhrafalh requested review froma team ascode ownersOctober 2, 2024 10:44
@cojencocojenco added the kokoro:force-runAdd this label to force Kokoro to re-run the tests. labelOct 4, 2024
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests. labelOct 4, 2024
Copy link
Contributor

@cojencocojenco left a 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 checksummd5 in the subsequent callblob._prep_and_do_download.@andrewsg thoughts/concerns to this change?

@rafalh Could you update the corresponding unit tests inhttps://github.com/googleapis/python-storage/blob/main/tests/unit/test_transfer_manager.py?

@andrewsg
Copy link
Contributor

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)
@rafalh
Copy link
ContributorAuthor

rafalh commentedOct 8, 2024
edited
Loading

I fixed the tests and got them to pass locally (executed onlytests/unit/test_transfer_manager.py). I had to make a change todownload_chunks_concurrently so it makes a copy ofdownload_kwargs dict argument before changing it (which is a good approach anyway). Otherwise some tests changedDOWNLOAD_KWARGS global variable and made other tests fail
Also removed unusedexpected_download_kwargs variables in some tests

@cojencocojenco added the kokoro:force-runAdd this label to force Kokoro to re-run the tests. labelOct 9, 2024
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests. labelOct 9, 2024
@cojencocojenco added the owlbot:runAdd this label to trigger the Owlbot post processor. labelOct 9, 2024
@gcf-owl-botgcf-owl-botbot removed the owlbot:runAdd this label to trigger the Owlbot post processor. labelOct 9, 2024
Copy link
Contributor

@cojencocojenco left a 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!

@cojencocojenco merged commit42392ef intogoogleapis:mainOct 9, 2024
1 check passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@cojencocojencocojenco approved these changes

Assignees

@andrewsgandrewsg

Labels

api: storageIssues related to the googleapis/python-storage API.size: xsPull request size is extra small.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

download_chunks_concurrently spams the log with INFO messages about checksum not being returned

4 participants

@rafalh@andrewsg@cojenco@yoshi-kokoro

[8]ページ先頭

©2009-2025 Movatter.jp