fix(storage): Handle redirect on takeover.#13354
Conversation
The initial takeover response wasn't being checked for a redirect. googleapis#13239 addressed successful responses. This addresses error responses.
product-auto-label
bot
added
the
api: storage
Summary of ChangesHello @cjc25, 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 a critical oversight in the storage system's gRPC stream takeover process. It ensures that redirect responses are properly handled even when an error occurs during the initial stream reception. This enhancement improves the robustness and reliability of the system by preventing unhandled redirects in error scenarios, complementing previous work that focused on successful responses. 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. 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
|
![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 correctly adds handling for redirection information in error responses during a takeover, which was a missing case. My review focuses on improving the consistency of the implementation with existing redirection handling logic in the success path, for better code maintainability.
| if err != nil { | ||
| s.streamErr = err | ||
| // A Recv() error may be a redirect. | ||
| s.streamErr = s.maybeHandleRedirectionError(err) |
There was a problem hiding this comment.
For consistency with how redirects are handled in the success path, consider changing maybeHandleRedirectionError to return a boolean. This would make the control flow more explicit and align the patterns used for success and error cases.
The existing success path handling in maybeHandleRedirection returns a boolean:
if s.maybeHandleRedirection(resp) {
s.streamErr = errRedirection
// ...
}Aligning this error path would look like this:
// A Recv() error may be a redirect.
if s.maybeHandleRedirectionError(err) { // This function would now return bool
s.streamErr = errRedirection
} else {
s.streamErr = err
}This would involve changing maybeHandleRedirectionError to return true when a redirect is successfully handled, and false otherwise. This approach separates the detection/handling of the redirect from the decision of which error to propagate, improving clarity.
tritone
left a comment
tritone
left a comment
There was a problem hiding this comment.
LGTM. I think this should go out in the patch release as well so that we get the append integration tests working correctly again.
|
kokoro failure looks like a read codec flake |
cjc25
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
PR created by the Librarian CLI to initialize a release. Merging this PR will auto trigger a release. Librarian Version: v0.6.0 Language Image: us-central1-docker.pkg.dev/cloud-sdk-librarian-prod/images-prod/librarian-go@sha256:718167d5c23ed389b41f617b3a00ac839bdd938a6bd2d48ae0c2f1fa51ab1c3d <details><summary>storage: 1.57.2</summary> ## [1.57.2](storage/v1.57.1...storage/v1.57.2) (2025-11-14) ### Features ### Bug Fixes * Handle redirect on takeover. (#13354) ([b0f1362](b0f13626)) * add env var to allow disabling bound token (#13236) ([cdaf6a6](cdaf6a6d)) ### Documentation * updates to docs and docs formatting (PiperOrigin-RevId: 828488192) ([93ca68d](93ca68d5)) </details>
3 participants
The initial takeover response wasn't being checked for a redirect.
PR #13239 addressed successful responses. This addresses error responses.
See #12785, b/459905219