thrift icon indicating copy to clipboard operation
thrift copied to clipboard

Overhaul the rust thrift library

Open reem opened this issue 9 years ago • 9 comments

The major changes are: - Concrete impls of Transport have been removed in favor of a blanket implementation. - Implement standard error handling idioms, including implementations of std::error::Error for all error types. - Various renames to bring things in line with idiomatic rust. - Change the Protocol trait to use static calls rather than virtual dispatch. - Allow Protocol's to mutate an internal state by changing the trait methods to receive &mut self. - Processor::process now receives &self ahead of a multithreaded server implementation.

This can't be landed as-is since there need to be accompanying changes to the code generator, but I wanted to solicit feedback on these changes first. I also have a multithreaded server ready to be PRed based on this branch, which you can see here.

reem avatar Jun 10 '15 21:06 reem

Apparently Github only shows the context for comments on the pull request itself, not the actual commits. That's annoying :/

Thanks for the pull request! Overall these changes look great to me, we'll wait for @maximg to chime in with any other comments.

I'm not sure about the Encode/Decode rename. While Read/Write aren't ideal either, they at least express that data is being written (and possibly sent) on the transport, not just encoded/decoded and waiting to be sent.

@maximg Can you push the benchmark you were using so we can see how changes like these affect performance?

gsingh93 avatar Jun 10 '15 22:06 gsingh93

Thanks for the PR! Will go over it this evening. Just committed the benchmark so feel free to compare.

maximg avatar Jun 11 '15 15:06 maximg

I am looking to make some changes to the code generator to merge these changes as well as add support for more of thrift, notably exceptions. However, I am a novice C++ programmer so it's taking me some time to become familiar with the generator - if either of you has a minute I'd love to chat a little about the code to help me get productive faster.

reem avatar Jun 11 '15 20:06 reem

Feel free to ping me on IRC.

gsingh93 avatar Jun 11 '15 20:06 gsingh93

For exceptions could you make sure to check with @gsingh93 - he is working on them already. I'll be happy to chat on skype (maxim.golov)

maximg avatar Jun 11 '15 20:06 maximg

I've pushed another commit adding a BufferedTransportServer wrapper which causes the yielded Transports to be wrapped in a BufStream. I think it makes sense to provide this as part of the library since it's a major performance gotcha.

reem avatar Jun 12 '15 02:06 reem

We may even want to integrate BufStream into the impl of TransportServer for TcpListener so the simple thing (passing a TcpListener as your TransportServer) does the right thing.

reem avatar Jun 12 '15 02:06 reem

The buffered transport looks good, but I don't think it's necessary to add it to the TcpListener impl. We're going to eventually follow the same pattern as the other libs and uses a factory (example), which will require the user to pass in a factory instead of a transport. The most basic factory will be a buffered factory, so the user shouldn't run into any surprises with their stream not buffering.

Also, I think TransportServer should be changed backed to ServerTransport. This is what the other libraries use, as well as the original thrift whitepaper. It does make sense, as it's the transport the server uses to talk to the client, it isn't the server itself (although both interpretations somewhat make sense).

gsingh93 avatar Jun 12 '15 03:06 gsingh93

Ok, I can change it back.

reem avatar Jun 12 '15 03:06 reem