fix(direct path verification): Updating direct path enforcement strategy#4635
Conversation
github-actions
Bot
added
the
remind-reviewers
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 refines the direct path enforcement strategy for storage client creation. It introduces conditional logic to handle zonal versus non-zonal buckets differently, ensuring that zonal buckets do not block client creation upon verification failure, while non-zonal buckets maintain strict enforcement. Additionally, the verification timeout has been significantly increased to improve the reliability of the detection process. 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
|
![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 increases the DirectPath detection timeout to 15 seconds and makes DirectPath verification failures fatal for regional buckets. The review feedback identifies an inconsistency between the new timeout and the maximum retry duration that could lead to premature cancellation, suggests refining log messages to correctly reflect the execution flow when verification fails, and recommends removing a redundant parameter in the gRPC client creation logic.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4635 +/- ##
==========================================
+ Coverage 83.62% 83.66% +0.04%
==========================================
Files 164 164
Lines 20278 20279 +1
==========================================
+ Hits 16958 16967 +9
+ Misses 2678 2673 -5
+ Partials 642 639 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…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.
…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.
* feat: making direct-path verification non-fatal until dummy-stat calls 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 * fix(buffered_reader): Fixing a memory leak in buffered reader (#4638) 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. * fix(direct path verification): Updating direct path enforcement strategy (#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. * feat(dp-check): removing older cp check utility (#4494) * feat(dp-check): removing older cp check utility * validation package is already disabled hence removing the skip --------- Co-authored-by: Prince Kumar <princer@google.com> Co-authored-by: Abhishek Gupta <abhishekmgupta@google.com>
2 participants
Description
Updating the direct path enforcement strategy:
Also, updated the timeout to 15 seconds for direct path verification
Link to the issue in case of a bug fix.
Testing details
Any backward incompatible change? If so, please explain.