XML MPU v/s Resumable Upload #1702
Conversation
product-auto-label
bot
added
size: m
Summary of ChangesHello @chandra-siri, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the capability to perform XML Multipart Uploads (MPU) for Google Cloud Storage objects. This new feature allows for breaking down large files into smaller, manageable parts that can be uploaded concurrently, offering a more robust and potentially faster upload experience. It enhances the flexibility of the upload process by supporting file-like objects as data sources and provides a configurable option to choose between XML MPU and traditional resumable uploads. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces changes to support XML Multipart Uploads (MPU) from file-like objects and adds a new XML MPU upload path in the Blob class, seemingly for comparison against resumable uploads. While the changes to XMLMPUPart to accept a file_obj are a good addition, the implementation within blob.py contains several issues. There are debugging print statements, incorrect comments, and most critically, the main upload logic in _do_upload has been replaced with benchmarking code that is not production-ready. My review includes several suggestions to clean up the code and a critical comment to address the benchmarking implementation before this can be merged.
| container.register_part(part_num, etag) | ||
|
|
||
| part_number += len(chunk_batch) | ||
| print("num parts uploaded:", part_number - 1) |
There was a problem hiding this comment.
This print statement appears to be for debugging. It should be removed from production code. Consider using the logging module if this information is valuable for diagnostics.
| print("num parts uploaded:", part_number - 1) | ||
|
|
||
| res = container.finalize(transport) | ||
| print("MPU Complete Response:", res) |
There was a problem hiding this comment.
This print statement appears to be for debugging. It should be removed from production code. Consider using the logging module if this information is valuable for diagnostics.
| if (filename is None and file_obj is None) or ( | ||
| filename is not None and file_obj is not None | ||
| ): |
There was a problem hiding this comment.
This condition can be simplified for better readability. A more Pythonic way to express that exactly one of filename or file_obj must be provided is if (filename is None) == (file_obj is None):.
| import time | ||
| import base64 | ||
| from concurrent.futures import ThreadPoolExecutor | ||
| import concurrent.futures |
There was a problem hiding this comment.
This import is redundant. ThreadPoolExecutor is already imported from concurrent.futures on the previous line, and concurrent.futures itself is not used directly in this file. You can remove this line.
| "storageClass": "x-goog-storage-class", | ||
| } | ||
|
|
||
| XML_CHUNK_SIZE = 100 * 1024 * 1024 # 8 MiB |
There was a problem hiding this comment.
The comment here is incorrect. 100 * 1024 * 1024 is 100 MiB, not 8 MiB. Please update the comment to reflect the correct size.
| XML_CHUNK_SIZE = 100 * 1024 * 1024 # 8 MiB | |
| XML_CHUNK_SIZE = 100 * 1024 * 1024 # 100 MiB |
| """ | ||
| 1. This should initialize XMLMPUContainer, container.initate(), you can keep filename as None. | ||
| 2. read chunks of data from stream, read at most `n` chunks (even if the file_stream is more, hold the stream there) | ||
| Where each `chunk_size` is provided as `XML_CHUNK_SIZE` | ||
| 3. Spawn multiple threads to upload each chunk using | ||
| part = XMLMPUPart() | ||
| part.upload() -> | ||
| each part upload should return (part_number, etag) | ||
| store these part numbers in a list/dictionary | ||
| using `container.register_part(part_number, etag)` | ||
| 4. read further chunks from stream and repeat step 3 until stream is exhausted | ||
| 5. Once all parts are uploaded, call | ||
| `container.finalize(blob._get_transport(client))` | ||
| to complete the multipart upload | ||
| """ |
There was a problem hiding this comment.
The docstring for this method appears to be a set of implementation notes. It should be converted into a proper docstring that explains what the method does, its parameters (file_obj, retry, content_type, num_of_threads), and what it returns. This will improve maintainability and make the code easier to understand for other developers.
1 participant
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 #<issue_number_goes_here> 🦕