-
Notifications
You must be signed in to change notification settings - Fork 154
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
docs: Add snippets for upload_chunks_concurrently and add chunk_size #1135
Conversation
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
@@ -0,0 +1,56 @@ | |||
# Copyright 2022 Google LLC |
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.
2023
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.
Will fix
# the use case. The remote service has a minimum of 5 MiB and a maximum of | ||
# 5 GiB. | ||
# chunk_size = 32 * 1024 * 1024 (32 MiB) | ||
|
||
# The maximum number of processes to use for the operation. The performance |
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.
Is the rule of thumb here: "number of cores your CPU has"?
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.
No, workloads with small files benefit from many times that number and workloads with large files max out the NIC below that, so the number of cores is not a good starting place.
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.
Is this something that would go into docs instead of sample comments?
BLOB_NAME = "test_file.txt" | ||
|
||
with tempfile.NamedTemporaryFile() as file: | ||
file.write(b"test") |
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.
this doesn't exercise multiple chunks; recommend increasing the object size to test chunk_size
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.
It's sufficient to test the snippet. The feature itself is not under test here - it is fully covered in the integration tests, with appropriately-sized test files.
source_filename, | ||
destination_blob_name, | ||
chunk_size=32 * 1024 * 1024, | ||
processes=8, |
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.
workers?
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.
Yeah, it might be easier to understand as "workers" for this and the other samples, I'll change it.
55448bb
to
18faaf1
Compare
Adds a snippet for upload_chunks_concurrently; also adds chunk_size to an existing snippet, and runs snippets_test.py through black for formatting purposes.