-
Notifications
You must be signed in to change notification settings - Fork 155
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
feat: add transfer_manager.upload_chunks_concurrently using the XML MPU API #1115
Conversation
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.
Looking good 🎉 have a few questions
Added integration tests. PTAL |
PTAL once more; this should be the final feature set I think |
filename, | ||
blob, | ||
content_type=None, | ||
chunk_size=TM_DEFAULT_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.
this is technically MPU part_size? Are using chunk_size for consistency?
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 is the part size. I am using chunk_size for consistency with the similar download method.
the documentation at https://cloud.google.com/storage/docs/multipart-uploads | ||
before using this feature. | ||
|
||
The library will attempt to cancel uploads that fail due to an exception. |
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.
+1
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.
Are you planning to add the upload_sharded
command name for metrics on in an other PR?
@danielduhh Yes, actually that's already ready for review and I'll have it up as soon as this one is reviewed and merged. |
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, just have two questions
@@ -1697,7 +1697,7 @@ def _get_writable_metadata(self): | |||
|
|||
return object_metadata | |||
|
|||
def _get_upload_arguments(self, client, content_type): | |||
def _get_upload_arguments(self, client, content_type, filename=None): |
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 just a reminder there are changes in the tm-metrics branch relevant to this private method as well
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.
Yep, already resolved via merge in a different branch
headers = {} | ||
# Handle standard writable metadata | ||
for key, value in metadata.items(): | ||
if key in METADATA_HEADER_TRANSLATION: |
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.
QQ are there any writable metadata fields that don't require translation?
Just wanted to make sure that they are all captured in _headers_from_metadata. If not, this works!
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 believe this is the complete list according to the documentation. Note that custom metadata and some other features like encryption are not supported through this translation dictionary but handled differently.
This adds support for XML MPU multiprocess uploads to Transfer Manager.