feat(dp-check): removing older cp check utility#4494
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 focuses on improving the reliability of direct path detection by deprecating an existing, less dependable client-side mechanism. The changes involve removing the associated code and temporarily disabling an end-to-end test that relied on this functionality, in anticipation of a new, more reliable server-side direct path check. This ensures a smoother transition to an enhanced direct path validation 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
|
There was a problem hiding this comment.
Code Review
This pull request removes an older, unreliable DirectPath connectivity check utility. The changes include removing the gRPCDirectPathDetector struct, its associated methods, and its usage within the storageClient. Consequently, related tests have been removed or skipped. Specifically, the TestNewStorageHandleDirectPathDetector unit test is deleted, and the TestGRPCDirectPathConnections e2e test is now skipped, as it relied on log messages from the removed utility. The changes are clean and align with the goal of phasing out the less reliable check in favor of existing, more robust verification mechanisms.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4494 +/- ##
==========================================
- Coverage 83.73% 83.69% -0.04%
==========================================
Files 163 163
Lines 20021 20077 +56
==========================================
+ Hits 16764 16804 +40
- Misses 2640 2649 +9
- Partials 617 624 +7
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:
|
|
Hi @Tulsishah, @charith87, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
* feat(dp-check): removing older cp check utility * validation package is already disabled hence removing the skip
* 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>
Description
Link to the issue in case of a bug fix.
b/493758597
Testing details
Any backward incompatible change? If so, please explain.