-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Initial simple grpc channel implementation #2338
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
Conversation
The MSRV check fails because we are using rust 1.86 features and the tonic repo uses a rust version of 1.75. Is there a way to work around this? I tried to set the rust version in the grpc |
@dfawley I don't see an easy to scope the msrv check, so I think its fine to bump it to 1.86 for now. I don't expect any msrv issues with the next release of tonic and in around 3 months the msrv will be valid. We just have a 6 month policy for msrv following what tokio does. |
@@ -6,19 +6,33 @@ authors = ["gRPC Authors"] | |||
license = "MIT" | |||
|
|||
[dependencies] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a follow up we should relax the versions required here unless we need a feature in a specific patch.
@@ -22,19 +22,119 @@ | |||
* | |||
*/ | |||
|
|||
use std::{any::Any, str::FromStr, sync::Arc}; | |||
use core::panic; | |||
use std::{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import style differs from what we do in the rest of tonic, we should think about aligning that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no strong feelings on this - just let me know how to do it and I can try to fix everything up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a very light look, overall LGTM, I expect we will have more review opportunity going forward when we change code. I also plan to work through this code locally when I get some time. But I think its better to merge this so others can work on top of it.
This is a minimum implementation of the grpc channel with a functional, pluggable name resolver, LB policy, and transport implementation. It includes examples that use an inmemory transport to send and receive messages. It is still in a very incomplete state, and still needs more review, refining, and tests to be written, and API changes are still expected. But I would like to merge it into the tonic repo in this approximate state to make ongoing development across the team easier than continuing to add code into my fork.