impl(storage): handle redirect errors#3715
Conversation
In the bidi streaming protocol the stream may be closed to redirect the client to a new handle and routing header. This PR introduces a helper function to convert a `grpc::Status` to `gax::error::Error` **and** also update the read spec (part of the initial bidi read request) with any redirect data. Parsing the error requires several storage-specific types. It seems better to keep this in the storage library.
|
/FYI: @BenWhitehead @cjc25 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3715 +/- ##
==========================================
+ Coverage 96.15% 96.17% +0.02%
==========================================
Files 131 132 +1
Lines 5226 5234 +8
==========================================
+ Hits 5025 5034 +9
+ Misses 201 200 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dbolduc
left a comment
There was a problem hiding this comment.
I have some questions. I don't have anything close to the full picture in my head.
| if let Ok(redirect) = d.to_msg::<BidiReadObjectRedirectedError>() { | ||
| let mut guard = spec.lock().expect("never poisoned"); | ||
| guard.routing_token = redirect.routing_token; | ||
| guard.read_handle = redirect.read_handle; |
There was a problem hiding this comment.
break?
I assume we expect exactly one BidiReadObjectRedirectedError.
There was a problem hiding this comment.
Done. I don't think we expect more than one and we save a wee-bit of time.
| guard.routing_token = redirect.routing_token; | ||
| guard.read_handle = redirect.read_handle; |
There was a problem hiding this comment.
Q: just checking: None is supposed to override the current value in spec?
What does None even mean?
There was a problem hiding this comment.
I assume that a redirect with a None means: "go back to the CFE and start over from just the bucket and object name". Possibly it is a "can't happen" thing, but I figured we would handle the case.
There was a problem hiding this comment.
@cjc25 in case he has opinions or thoughts.
There was a problem hiding this comment.
tl;dr: clearing in the request is fine if either field in the error is None.
The service should never set an empty routing token in practice, but if it does, then clearing the field in the request (and the x-goog-request-params header) is fine and we will eventually route to an endpoint that can serve the request.
The service should never set read_handle to empty in a redirect error if it already returned a read_handle in some other response. It may return an empty read_handle if no response was ever delivered. Regardless, it is always safe, but slower, to send a read retry without a read_handle in the request, so clearing here if we hit that case is safe.
In the bidi streaming protocol the stream may be closed to redirect the client to a new handle and routing header. This PR introduces a helper function to convert a
grpc::Statustogax::error::Errorand also update the read spec (part of the initial bidi read request) with any redirect data.Parsing the error requires several storage-specific types. It seems better to keep this in the storage library.
Part of the work for #3626