Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP transport for Thrift #125

Open
ii64 opened this issue Feb 1, 2023 · 10 comments
Open

HTTP transport for Thrift #125

ii64 opened this issue Feb 1, 2023 · 10 comments
Assignees
Labels
A-volo-thrift The issue concerns the `volo-thrift` crate. C-proposal A proposal of some kind, and a request for comments. E-help-wanted Stuff where we want help.

Comments

@ii64
Copy link
Member

ii64 commented Feb 1, 2023

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, implements volo::net::dial::MakeTransport.

Alternatives

RFC

@PureWhiteWu
Copy link
Member

Is there a spec that describes the HTTP transport?
I didn't find it.

@ii64
Copy link
Member Author

ii64 commented Feb 7, 2023

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

@PureWhiteWu
Copy link
Member

Maybe we should submit an issue for that to the Apache thrift repo?

@ii64
Copy link
Member Author

ii64 commented Feb 20, 2023

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?

@PureWhiteWu
Copy link
Member

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:

  1. We don't know the HTTP transport spec.
  2. Is anyone using the HTTP transport now?

@PureWhiteWu PureWhiteWu added A-volo-thrift The issue concerns the `volo-thrift` crate. C-proposal A proposal of some kind, and a request for comments. E-help-wanted Stuff where we want help. labels Feb 20, 2023
@thaodt
Copy link

thaodt commented Oct 25, 2023

hi, do we still need it? If yes, I want to give it a try

@ii64
Copy link
Member Author

ii64 commented Oct 25, 2023

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

@ii64
Copy link
Member Author

ii64 commented Dec 23, 2023

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 ii64 self-assigned this Dec 23, 2023
@thaodt
Copy link

thaodt commented Dec 25, 2023

@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.

@ii64
Copy link
Member Author

ii64 commented Dec 27, 2023

No worries, we are still doing RFC, you can check the discussion on linked PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-volo-thrift The issue concerns the `volo-thrift` crate. C-proposal A proposal of some kind, and a request for comments. E-help-wanted Stuff where we want help.
Development

No branches or pull requests

3 participants