feat: expose DoGet response headers & trailers #4727
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
arrow
crepererum
commented
Aug 23, 2023
Comment on lines
+70
to
+72
| http = "0.2.9" | ||
| http-body = "0.4.5" | ||
| pin-project-lite = "0.2" |
There was a problem hiding this comment.
These are not really new dependencies, they are already used by tonic & tower, but I need to name the types.
|
I can extend this to other client methods in follow-up PRs if this gets accepted. |
crepererum
force-pushed
the
crepererum/flight_get_header_trailer
branch
2 times, most recently
from
e8cd55c to
852651e
Compare
alamb
approved these changes
Aug 23, 2023
There was a problem hiding this comment.
This is both very cool and very nicely coded @crepererum -- thank you and well done ❤️
I had some small naming / comment suggestions, but otherwise 🚀
| @@ -204,16 +204,14 @@ impl FlightClient { | |||
| pub async fn do_get(&mut self, ticket: Ticket) -> Result<FlightRecordBatchStream> { | |||
There was a problem hiding this comment.
There are other methods on this client that might be interested in server headers/trailers -- like do_exchange and do_put. Maybe we can file a ticket to track adding support for them too
crepererum
force-pushed
the
crepererum/flight_get_header_trailer
branch
2 times, most recently
from
a239401 to
147ff80
Compare
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
crepererum
force-pushed
the
crepererum/flight_get_header_trailer
branch
from
147ff80 to
74ed1df
Compare
crepererum
force-pushed
the
crepererum/flight_get_header_trailer
branch
from
74ed1df to
8adf5e3
Compare
alamb
reviewed
Aug 25, 2023
|
|
||
| /// Extract [`LazyTrailers`] from [`Streaming`] [tonic] response. | ||
| /// | ||
| /// Note that [`LazyTrailers`] has inner mutability and will only hold actual data after [`ExtractTrailersStream`] is |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
-
Rationale for this change
HTTP/2 headers can already be sent by the client (and trivially by the server as part of the
tonic::Responseobject). However the the client doesn't expose the response headers yet. They can include debug information like server version or other metadata.Trailers are less commonly used but can include information that is only available AFTER the response stream finishes, e.g. "number of scanned files", "total rows read from files", etc. Tonic doesn't have a super nice way to write them from the server side, but a tower layer makes that possible. Also we have to consider non-Rust implementations that can make use of trailers.
People that read the HTTP/2 spec may wonder how headers and trailers are encoded in a stream scenario. Turns out that they have their own
HEADERSmessage. The firstHEADERSmessage transmit the HTTP headers, the last one at the stream end (w/ theEND_STREAMflag set) the trailers. Now you may wonder if you could send headers mid-stream as well. Now the HTTP/2 spec doesn't seem to disallow that, but:neither tonic and tower have a way to do that
this stackoverflow answer suggests that this is not allowed because HTTP itself has no semantic for that and indeed the HTTP/2 spec say:
This is also confirmed by this stackoverflow answer.
So I think metadata can only be sent at the beginning and at the end of a stream. Everything else would also be a bit of an API mess.
What changes are included in this PR?
Wires HTTP/2 headers and trailers into flight client
DoGetmethod. Tests included.Are there any user-facing changes?
New methods to get headers and trailers.