thrift
thrift copied to clipboard
maximg
Macros + Exceptions + Library Overhaul
Introduces a mostly entirely new architecture for code generation and overhauls most APIs.
The C++ code generator now acts as a thin front-end generating calls to rust macros defined in the thrift crate, which acts as a back-end and does the actual code generation at rust compile time.
The generated Encode and Decode impls are now recursive, so instead of recursively generating the entire encoding/decoding strategy for a type we now just (figuratively) call encode or decode on all the fields. In addition to major compilation-time advantages, this approach also allows for things like recursive structs.
Lastly, this PR includes an implementation of thrift exceptions. Because thrift exception implementations in other languages rely heavily on __isset and nullability, this included a change making all struct fields Options as briefly discussed in #14 and requires exposing the XXXXResult types to server implementors. cc @gsingh93 if you had other ideas here.
The rust server and client examples now interoperate fully with others, and I have tested them against the python and C++ servers and clients.
Also, a multithreaded server is included.
Sorry for slow review, this week is very busy with a house move. The general direction with moving more code into the library looks good.
Will try to have my comments by the end of the week. Thanks!
It turns out that with the new system of generating code (with rust macros), this is a bit harder to review as there's no diff of the changes to generated code.
I haven't had time to thoroughly go through all of this, but regarding exceptions, do you think we could check whether we got back an exception or a successful result while decoding, and based on that return a Result to the user? I don't like the fact that every field needs to be an option now.
@gsingh93 that is a perfectly reasonable next step - the challenge is that we have to generate a new error enum for each method, which I guess is not the end of the world.
I also don't particularly like the interface, I just needed exceptions so I implemented them in the simplest way that could re-use the most code. I imagine the macros actually could be modified to generate an error enum for each method - all the information you need is in the methods block already, so the C++ frontend doesn't need to change.
Sorry for the delay in reviewing, I just got around to it. Everything looks good, most of my comments are nits and questions. Did you test this with the servers/clients of other languages?
Also, can you rename TransportServer back to ServerTransport as we discussed in your previous PR?
BTW, Github is showing an extra commit by Maxim in your PR, I think you need to rebase.
Done with checking; it's a pity my knowledge of Rust macros does not allow to meaningfully comment on them. If you could take care of the comments will be happy to merge (now that my move is complete things should go a bit faster).
Thanks for this major contribution.
What's the state of this PR and library? Seems to have been largely abandoned for a month+ now.
Paul, I am away from the project at least until the end of September. Feel free to pick up one of the issues if you have time to help.
@reem, there were some comments on this PR, what was your plan for them?
I talked to @reem on IRC a few weeks ago. His fork at https://github.com/terminalcloud/thrift is much farther ahead than this repo, and I think it'd be a lot of effort to get all of those changes reviewed and merged here. And I think that fork is currently being used in production, so it's much more tested.
It might make more sense to consider that fork as the new "main" repository. Our main goal is to get thrift merged into the mainline, and I think wasting time trying to merge all those changes here would go against that goal.
I'd still like to see these comments addressed in reem's fork though.
That's good news, I think @reem is also in a better position to maintain the new codebase.
On Wed, Sep 16, 2015 at 8:12 PM, Gulshan Singh [email protected] wrote:
I talked to @reem https://github.com/reem on IRC a few weeks ago. His fork at https://github.com/terminalcloud/thrift is much farther ahead than this repo, and I think it'd be a lot of effort to get all of those changes reviewed and merged here. And I think that fork is currently being used in production, so it's much more tested.
It might make more sense to consider that fork as the new "main" repository. Our main goal is to get thrift merged into the mainline, and I think wasting time trying to merge all those changes here would go against that goal.
I'd still like to see these comments addressed in reem's fork though.
— Reply to this email directly or view it on GitHub https://github.com/maximg/thrift/pull/41#issuecomment-140826405.