grpc: implement Handle for servers and remove old Service abstraction#2536
grpc: implement Handle for servers and remove old Service abstraction#2536dfawley merged 5 commits intohyperium:masterfrom
Conversation
|
We can also delete the tonic codec used by the tonic transport: https://github.com/hyperium/tonic/blob/master/grpc/src/codec.rs |
Done, thanks! |
dfawley
force-pushed
the
generic_api3
branch
2 times, most recently
from
b7b8bfe to
4bd10dd
Compare
| pub(crate) mod attributes; | ||
| pub(crate) mod byte_str; | ||
| pub(crate) mod rt; | ||
| pub(crate) mod send_future; |
There was a problem hiding this comment.
Using pub(crate) in the root lib.rs file is essentially the same as making the module private. In both cases, only descendants of lib.rs can access it. I removed the pub(crate) modifiers in a previous PR.
There was a problem hiding this comment.
Private mods are the same as pub(crate) mods? TIL. Changed.
| && service_config | ||
| .load_balancing_policy | ||
| .as_ref() | ||
| .is_some_and(|p| *p == LbPolicyType::RoundRobin) |
There was a problem hiding this comment.
Depending on whether we want to allow custom LBs, I can think of two better ways:
- Use the LbPolicyType enum as the key to the
GLOBAL_LB_REGISTRY, or - Have a function that maps
LbPolicyTypeto an&'static str
The seems to be easier to extend when we add more LBs.
There was a problem hiding this comment.
I was "implementing support for the loadBalancingPolicy field" with this:
Apparently Java supports this being any string, so @nathanielford will update this later to work similarly.
But this is all here just so I can do RR in the example.
dfawley
left a comment
dfawley
left a comment
There was a problem hiding this comment.
Thanks for the review! PTAL
| pub(crate) mod attributes; | ||
| pub(crate) mod byte_str; | ||
| pub(crate) mod rt; | ||
| pub(crate) mod send_future; |
There was a problem hiding this comment.
Private mods are the same as pub(crate) mods? TIL. Changed.
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
On major comment about making internal modules pub in lib.rs, otherwise LGTM.
| pub mod attributes; | ||
| pub mod byte_str; | ||
| pub mod rt; | ||
| pub mod send_future; |
There was a problem hiding this comment.
These should not be pub, simply mod.
2 participants
Note: This PR is based on top of #2528, so review can wait until after that is approved (or close to ready).
Also:
inmemoryexample to use new abstractions.multiaddrexample; updateinmemoryexample to handle multiple addresses instead (there was a lot of code duplication).The
inmemorytransport implementation is doing some fishy things right now. But "it works". It's actually covering up bugs in the subchannel implementation that are, I believe, holding circular references between internal/external subchannels and preventing proper shutdown from occurring. This is a pretty major issue, but it needs to be solved before the inmemory transport can be fixed. I will start working on that problem next, before dealing with the protobuf codegen for clients.