fix(client): release vectored read buffers on failure - #313
Conversation
Summary of ChangesHello, 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 improves the reliability of vectored read operations by ensuring that allocated buffers are properly released when a read operation fails. By threading a release callback through the core and client implementations, the system prevents potential memory leaks associated with pooled or direct ByteBuffers, aligning the API with modern Hadoop vectored read standards. Highlights
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 the 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 counterproductive. 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. 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 a new overload for the readVectored method across several classes and interfaces, including VectoredSeekableByteChannel, GcsReadChannel, SeekableInputStream, and GoogleCloudStorageInputStream, to support a buffer release callback on failure. It also implements the corresponding release logic and adds comprehensive unit tests. The reviewer identified a critical issue in GoogleCloudStorageInputStream.readVectored where the buffer allocation is performed outside of the try-catch block; if the allocator throws an exception, it will propagate immediately and leave subsequent futures uncompleted, potentially causing threads to hang.
…nside the try block, only releases when a buffer was actually allocated, and completes the range future exceptionally on allocator failure. Added a regression test covering allocator failures across multiple cached ranges to ensure no futures are left incomplete.
|
@gemini-code-assist initiate re-scan |
|
Hello @arnabnandy7, I do not have a specific "re-scan" command. However, if you would like me to perform a fresh code review of the current state of your pull request, you can simply use the |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an overloaded readVectored method across several classes (including GcsReadChannel and GoogleCloudStorageInputStream) to support a Consumer<ByteBuffer> release callback, ensuring allocated buffers are properly cleaned up on failure. It also adds comprehensive unit tests to validate this behavior. The review feedback recommends avoiding hardcoded magic numbers like 1024 in the new test cases, suggesting the use of named constants instead to improve readability and maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new overload for the readVectored method across VectoredSeekableByteChannel, GcsReadChannel, SeekableInputStream, and GoogleCloudStorageInputStream that accepts a Consumer release callback. This callback is used to release allocated ByteBuffer instances on failure, preventing potential memory leaks. The changes also include corresponding unit tests to verify the release behavior under various success and failure scenarios, as well as refactoring hardcoded size constants in tests. There are no review comments, and I have no additional feedback to provide.
…failure with optimizer architecture
|
Several pull requests have recently been merged into main. Could you please sync your branch with main and resolve any outstanding conflicts? |
Sure I see there's a conflict, I'll resolve it. |
…ad-api # Conflicts: # core/src/main/java/com/google/cloud/gcs/analyticscore/core/optimizer/SmallObjectOptimizer.java
|
@prudhvimaharishi please review. I've fixed the conflict |
|
Can you please sync again. |
|
@prudhvimaharishi done |
2 participants
Type of Change
feat: A new featurefix: A bug fixdocs: Documentation only changesstyle: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)refactor: A code change that neither fixes a bug nor adds a featureperf: A code change that improves performancetest: Adding missing tests or correcting existing testschore: Changes to the build process or auxiliary tools and libraries such as documentation generationDescription
What?
Related to #174
Add backward-compatible
readVectored(..., allocate, release)overloads and thread the release callback through the core and client vectored read implementations.Buffers allocated during vectored reads are now released when a read fails before the range futures are completed exceptionally. Tests cover release-on-failure, no release on successful reads, no release when allocation itself fails, and cached small-object vectored read failures.
Why?
This prevents leaked pooled or direct
ByteBufferinstances when vectored reads fail after allocation, aligning the API with newer Hadoop vectored read behavior.Checklist
feat(core): ...)Generated/Assisted by Agent? Yes