jorgecarleitao commented on pull request #2426: URL: https://github.com/apache/thrift/pull/2426#issuecomment-890551249
Thanks! > A couple of high-level thoughts from skimming through the code. I wonder if there's a better way of generalizing the functions than adding an is_sync flag everywhere and repeating the is_sync block in multiple places to add the async prefix and use .await?. There are a couple of places where we now call a function with multiple boolean args, and without keyword args it can get confusing. I have no strong opinions here: the approach here just avoids code duplication by branching of with the `is_sync`. I personally have no problem with these arguments, but happy to change it to whatever makes sense to you. > Finally, I've never used async-trait. Do you know if the Thrift generator requires any features that are not supported by it? `async-trait` is just a macro to enable traits with `async`. We have been using it in Apache Arrow (e.g. [here](https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/physical_plan/hash_aggregate.rs#L194)) to declare a trait that is `async`. `integer-encoding`, which we depend on, also uses it when compiled with `futures_async`. The declarations do not need this, only the lib declaring and implementing the trait. > Next, I would like to see a rough cut of how TInputStreamProtocol is implemented, just to see how everything fits together. I agree. I have been working on it today. I just pushed a draft to this. Still not compiling, but it gives a general idea; it is just mundane code of the equivalent `async` of the `TCompactInputProtocol`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
