Open ii64 opened 1 year ago
Is there a spec that describes the HTTP transport? I didn't find it.
Me neither, people were able to implement them in any language and do cross-test between each of the implementation so I think there should be a general idea. I agree that we need a spec about that. Apache Thrift had it supported since 0.1.x https://github.com/apache/thrift/blob/0.1.x/lib/cpp/src/transport/THttpClient.cpp
Maybe we should submit an issue for that to the Apache thrift repo?
apache/thrift repository seem don't accept issue from GitHub, Apache's Jira public signup is also disabled (https://infra.apache.org/jira-guidelines.html#who).
Theoretically speaking, can HTTP Transport implemented with only current impl MakeTransport
?
I've looked into this a bit, impl MakeTransport
has:
async fn make_transport(&self,addr:volo::net::Address) -> std::io::Result<(Self::ReadHalf,Self::WriteHalf)>
And think that it's not the correct way of open a connection (over HTTP) since it need to be AsyncWrite
and AsyncRead
at the same time, AsyncWrite
has poll_flush
but it can't be implemented in there since there will be a new HTTP response Body
to be consumed by the Decoder
.
This may need to implement another Client beside existing multiplex and pingpong, the motore::{Service,UnaryService}
. What do you think about this?
I'm not quite sure if we can implement HTTP transport using the current abstraction, but I think we can try it.
For the server part, I think we need an underlying HTTP server which handle the HTTP protocol and exposes a read loop and write loop as the ReadHalf
and WriteHalf
, which sends and receives the encoded message(but do we need to add framed or ttheader here?).
For the client part, this may be simpler than server, we can use a mature http client(such as reqwest) and use its own connection pool.
I haven't thought about this carefully, so I'm not sure this will work.
The biggest problems now are that:
hi, do we still need it? If yes, I want to give it a try
Absolutely, that would be fantastic! We can figure out later on how we are going to merge the feature. Your offer of help is greatly appreciated. Thanks
Hi, please kindly check the PR.
Currently the implementation is using reqwest
and only client. I'd like to see this stabilized on volo
and using our abstraction on volo-http
but aside from that, it is pretty much functioning and I wonder if we can roll-out that as we've proposed the public API related for HTTP transport.
E.g.
let url = "https://127.0.0.1/thrift-http-endpoint".parse::<hyper::Uri>().unwrap();
let url = volo::net::Address::from(url);
let client = volo_thrift::client::ClientBuilder::
new("dummy", dummys::MkDummyServiceGenericClient)
.make_codec(mk_codec)
.address(url)
.header("user-agent", "volo-http/1.0".parse().unwrap())
.build();
More detailed example can be accessed on https://github.com/ii64/volo_testing/blob/f796b7360e32493b450f907d27a048c936261b1d/the_example/src/bin/client_http_testing.rs
@ii64 at a first glance, it seems great. Sorry for lack of activity for a long time. But im curious on your implementation, I will check your PR also.
No worries, we are still doing RFC, you can check the discussion on linked PR
Feature Request
Support for HTTP server/client transport wrapper for Thrift RPC.
Crates
volo-thrift
Motivation
Extending compatibility of Thrift implementation to support HTTP transport wrapper. Current state of supported features compared with another Thrift library: apache/thrift/LANGUAGES.md
Proposal
Using
hyper
as Thrift HTTP transport wrapper, implementsvolo::net::dial::MakeTransport
.Alternatives
RFC