feat(patch): Patch fixes 3.8.2#4659
Conversation
…s becomes reliable (#4628) * feat: add skipDirectPathEnforcement parameter to createGRPCClientHandle to allow conditional DirectPath enforcement * chore: increase directPathDetectionTimeout from 10 seconds to 5 minutes * simplifying a bit * making direct-path verification non-fatal * removing the timeout from the client creation * minor change * removing empty line
Fixes a memory leak in the buffered reader code during multi-block reads. Previously, if an initial block downloaded successfully but a later block failed (triggering a fallback), the reference counts on the successfully downloaded blocks were never decremented. This PR addresses the leak by catching gcsx.FallbackToAnotherReader errors and calling a new releaseInflightBlocks helper to immediately invoke callbacks and release references for any in-flight blocks.
…egy (#4635) Updating the direct path enforcement strategy: - For zonal buckets, log direct path verification status but do not block client creation on it - For non-zonal buckets, block grpc client creation on direct path verification status. In case of failure in detecting direct path status, fallback to http client would happen based on the set grpc-strategy Also, updated the timeout to 15 seconds for direct path verification.
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 addresses critical stability and resource management issues. It fixes a memory leak in the buffered reader during fallback scenarios and improves the robustness of DirectPath connectivity verification for GCS clients. These changes ensure better resource cleanup and more reliable client initialization. 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 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the ReadAt method in BufferedReader to manage resource cleanup via a defer block and introduces a new test case for fallback scenarios. It also modifies DirectPath connectivity logic in storage_handle.go, increasing detection timeouts and making verification non-fatal for zonal buckets. Feedback highlights potential resource leaks in ReadAt for general errors and in createGRPCClientHandle due to a missing sc.Close() call on verification failure. Additionally, it is suggested to restore the explicit timeout for NewGRPCClient to ensure fail-fast behavior.
* feat(dp-check): removing older cp check utility * validation package is already disabled hence removing the skip
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the ReadAt method in buffered_reader.go to manage block reference releases via a defer block and removes the gRPCDirectPathDetector in storage_handle.go, making DirectPath verification fatal only for regional buckets. Review feedback identified a potential memory leak in ReadAt where block references might not be released for errors other than FallbackToAnotherReader. Additionally, a resource leak was found in createGRPCClientHandle where the gRPC client is not closed when verification fails for regional buckets.
abhishek10004
left a comment
There was a problem hiding this comment.
LGTM (Checked with Pranjal and these commits were cherry picked)
|
Merging this for now (need to test something on louhi/run this incase no issues). |
Patch fixes
direct path verification (changes across 3 PRs) -