fix(storage): Handle request transformation after gRPC BidiWriteObject redirects#16073
fix(storage): Handle request transformation after gRPC BidiWriteObject redirects#16073kalragauri wants to merge 1 commit intogoogleapis:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the propagation of routing tokens across retries for asynchronous appendable uploads. It introduces a RoutingHeaderOptions structure to manage tokens in gRPC metadata and adds the HandleBidiWriteRedirect utility to centralize redirect handling and request specification updates. Feedback identifies that the call to HandleBidiWriteRedirect within AsyncWriterConnectionImpl::OnQuery is ineffective, as the modified request state is not preserved or propagated back to the higher-level retry loop in AsyncConnectionImpl.
| EnsureFirstMessageAppendObjectSpec(request_, grpc_status); | ||
| ApplyWriteRedirectErrors(*request_.mutable_append_object_spec(), | ||
| grpc_status); | ||
| HandleBidiWriteRedirect(request_, grpc_status); |
There was a problem hiding this comment.
The call to HandleBidiWriteRedirect here appears to have no effect. This function modifies the request_ member, but OnQuery is a destructive operation that closes the stream, rendering this AsyncWriterConnectionImpl instance unusable. The retry logic that would benefit from the modified request exists at a higher level (in AsyncConnectionImpl) and creates a new writer connection for each attempt. The state from this failed connection, including the modified request_, is not propagated.
Since the modifications to request_ are lost, this call can be removed. Note that removing this line will cause the grpc_status variable on the preceding line to be unused, so that line should probably be removed as well.
18a62db to
30b6578
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16073 +/- ##
==========================================
+ Coverage 92.69% 92.71% +0.01%
==========================================
Files 2343 2343
Lines 216674 216989 +315
==========================================
+ Hits 200852 201172 +320
+ Misses 15822 15817 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| google::storage::v2::AppendObjectSpec const& spec, | ||
| RoutingHeaderOptions const& options) { | ||
| std::string params = | ||
| "bucket=" + google::cloud::internal::UrlEncode(spec.bucket()); |
There was a problem hiding this comment.
It appears the only difference between these two functions is whether or not to call spec.bucket() or spec.resource().bucket(). Considering the number of identical string literals that have to be correct in both functions. I would recommend factoring out a helper function that contains the common code.
| params += "&routing_token=" + | ||
| google::cloud::internal::UrlEncode(options.routing_token); | ||
| } | ||
| context.AddMetadata("x-goog-request-params", params); |
…Object redirects This commit addresses an issue where BidiWriteObject operations, particularly in cross-region redirect scenarios, would fail with a NOT_FOUND error. The root cause was that bucket information was not propagated in the request headers after the client received a redirect error from the service.
30b6578 to
56269bc
Compare
This commit addresses an issue where gRPC BidiWriteObject operations, particularly in cross-region redirect scenarios, would fail with a NOT_FOUND error. The root cause was that bucket information was not propagated in the request headers after the client received a redirect error from the service.