chore(storage): add chunking, composing and cleanup logic for PCU#13600
chore(storage): add chunking, composing and cleanup logic for PCU#13600cpriti-os wants to merge 6 commits intogoogleapis:mainfrom
Conversation
Summary of ChangesHello @cpriti-os, 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 significantly enhances the Parallel Composite Upload (PCU) functionality by integrating comprehensive data chunking, multi-level object composition, and robust cleanup mechanisms. These changes enable the system to efficiently handle large file uploads by breaking them into manageable parts, assembling them intelligently, and ensuring that temporary artifacts are properly managed, all while improving the overall testability of the component. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality for parallel composite uploads (PCU), including chunking data into parts, composing parts (recursively if needed), and cleaning up temporary objects. The changes are well-structured, using dependency injection for testability, and are accompanied by a comprehensive suite of unit tests covering many edge cases. The logic for concurrent part uploads, composition, and cleanup appears robust. I have two main points of feedback: a critical issue in generateRandomBytes where a potential error from crypto/rand.Read is ignored, which could lead to non-random data and object name collisions, and a high-severity issue in the close method's error handling, which could cause the cleanup logic to behave incorrectly on composition failures. After addressing these points, the implementation will be much more robust.
| // Acquire a buffer from the pool if we don't have one. | ||
| if s.currentBuffer == nil { | ||
| // Fail-fast check before taking a new buffer. | ||
| s.mu.Lock() |
There was a problem hiding this comment.
Can you move this logic to a function. This is repeated at multiple places.
There was a problem hiding this comment.
There are just three lines that can be moved to a function but it can only possibly replace three instances, I think adding a function for so small a logic will not add to readability.
|
initial review |
No region tags are edited in this PR.This comment is generated by snippet-bot.
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant new logic for parallel composite uploads, including asynchronous chunking, recursive composition for large objects, and robust cleanup of temporary parts. The changes are well-structured, leveraging goroutines, channels, and other concurrency primitives effectively. The addition of comprehensive unit tests for the new functionality is also a great inclusion.
I've identified a couple of issues related to error handling that could lead to bugs under specific failure conditions, and one minor anti-pattern in the test code. My feedback includes suggestions to address these points.
| b := make([]byte, n) | ||
| _, _ = rand.Read(b) | ||
| return b |
There was a problem hiding this comment.
The error returned by rand.Read is ignored. According to the documentation, crypto/rand can return an error if the underlying OS entropy source fails. Ignoring this error could result in the function returning a buffer of zeros, which would lead to predictable temporary object names and potential collisions, defeating the purpose of using a cryptographically secure random generator. This is a critical failure condition, and the application should probably panic to avoid operating in an insecure state.
b := make([]byte, n)
if _, err := rand.Read(b); err != nil {
// This is a catastrophic failure. The OS's entropy source is broken.
panic(fmt.Sprintf("crypto/rand.Read failed: %v", err))
}
return b
Asynchronous Chunking and Uploads
Recursive Composition for Large Objects
Robust Cleanup