-
Notifications
You must be signed in to change notification settings - Fork 1.2k
grpc/client: Implement Channel builder #2675
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
base: master
Are you sure you want to change the base?
Changes from all commits
4a29d46
6739db6
ef64d7c
b9bce2a
7f240f9
7365a7a
d2649c6
44f7e27
c0e1055
65b7738
4390322
45f16c0
8cf7952
c821541
2714d5b
e197877
0758d58
73c3ddf
8c1e34b
e98a9c9
2232440
fbcbbb1
308b1c7
e820a38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,17 +29,14 @@ | |
| use std::str::FromStr; | ||
| use std::sync::Arc; | ||
| use std::sync::Mutex; | ||
| use std::time::Duration; | ||
| use std::time::Instant; | ||
| use std::vec; | ||
|
|
||
| use serde_json::json; | ||
| use tokio::sync::mpsc; | ||
| use tokio::sync::watch; | ||
|
|
||
| use crate::StatusCodeError; | ||
| use crate::StatusError; | ||
| use crate::attributes::Attributes; | ||
| use crate::client::CallOptions; | ||
| use crate::client::ConnectivityState; | ||
| use crate::client::DynInvoke; | ||
|
|
@@ -84,71 +81,9 @@ | |
| use crate::credentials::common::Authority; | ||
| use crate::rt; | ||
| use crate::rt::GrpcRuntime; | ||
| #[cfg(feature = "_runtime-tokio")] | ||
| use crate::rt::default_runtime; | ||
|
|
||
| /// Configuration options for [`Channel`]s. | ||
| #[non_exhaustive] | ||
| pub struct ChannelOptions { | ||
| pub(crate) transport_options: Attributes, // ? | ||
| pub(crate) channel_authority: Option<String>, | ||
| pub(crate) connection_backoff: Option<Todo>, | ||
| pub(crate) default_service_config: Option<String>, | ||
| pub(crate) disable_proxy: bool, | ||
| pub(crate) disable_service_config_lookup: bool, | ||
| pub(crate) disable_health_checks: bool, | ||
| pub(crate) max_retry_memory: u32, // ? | ||
| pub(crate) idle_timeout: Duration, | ||
| // TODO: pub transport_registry: Option<TransportRegistry>, | ||
| // TODO: pub name_resolver_registry: Option<ResolverRegistry>, | ||
| // TODO: pub lb_policy_registry: Option<LbPolicyRegistry>, | ||
|
|
||
| // Typically we allow settings at the channel level that impact all RPCs, | ||
| // but can also be set per-RPC. E.g.s: | ||
| // | ||
| // - interceptors | ||
| // - user-agent string override | ||
| // - max message sizes | ||
| // - max retry/hedged attempts | ||
| // - disable retry | ||
| // | ||
| // In gRPC-Go, we can express CallOptions as DialOptions, which is a nice | ||
| // pattern: https://pkg.go.dev/google.golang.org/grpc#WithDefaultCallOptions | ||
| // | ||
| // To do this in rust, all optional behavior for a request would need to be | ||
| // expressed through a trait that applies a mutation to a request. We'd | ||
| // apply all those mutations before the user's options so the user's options | ||
| // would override the defaults, or so the defaults would occur first. | ||
| pub(crate) default_request_extensions: Vec<Todo>, // ?? | ||
| } | ||
|
|
||
| impl Default for ChannelOptions { | ||
| fn default() -> Self { | ||
| Self { | ||
| transport_options: Attributes::default(), | ||
| channel_authority: None, | ||
| connection_backoff: None, | ||
| default_service_config: None, | ||
| disable_proxy: false, | ||
| disable_service_config_lookup: false, | ||
| disable_health_checks: false, | ||
| max_retry_memory: 8 * 1024 * 1024, // 8MB -- ??? | ||
| idle_timeout: Duration::from_secs(30 * 60), | ||
| default_request_extensions: vec![], | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl ChannelOptions { | ||
| /// Overrides the channel authority, which is used for credentials | ||
| /// handshaking and HTTP virtual hosting. | ||
| pub fn override_authority(self, authority: impl Into<String>) -> Self { | ||
| Self { | ||
| channel_authority: Some(authority.into()), | ||
| ..self | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// A virtual, persistent connection to a gRPC service. | ||
| /// | ||
| /// A `Channel` begins in an "idle" state and connects when the first RPC is | ||
|
|
@@ -161,33 +96,29 @@ | |
| } | ||
|
|
||
| impl Channel { | ||
| /// Constructs a new gRPC channel. Channel creation cannot fail, but if the | ||
| /// target string is invalid, the returned channel will never connect, and | ||
| /// will fail all RPCs. | ||
| pub fn new( | ||
| /// Creates a new channel builder for the given target. | ||
| ///# Example | ||
| /// | ||
| /// ``` | ||
| /// use std::sync::Arc; | ||
| /// use grpc::client::Channel; | ||
| /// use grpc::credentials::LocalChannelCredentials; | ||
| /// | ||
| /// let channel = Channel::builder("dns:///localhost:123", Arc::new(LocalChannelCredentials::new())) | ||
| /// .build(); | ||
| /// ``` | ||
| #[cfg(feature = "_runtime-tokio")] | ||
| pub fn builder( | ||
| target: impl Into<String>, | ||
| credentials: Arc<dyn ChannelCredentials>, | ||
| options: ChannelOptions, | ||
| ) -> Self { | ||
| pick_first::reg(); | ||
| round_robin::reg(); | ||
| dns::reg(); | ||
| #[cfg(unix)] | ||
| name_resolution::unix::reg(); | ||
| #[cfg(target_os = "linux")] | ||
| name_resolution::unix_abstract::reg(); | ||
| #[cfg(feature = "_runtime-tokio")] | ||
| tonic_transport::reg(); | ||
| Self { | ||
| inner: Arc::new(PersistentChannel::new( | ||
| target, | ||
| default_runtime(), | ||
| options, | ||
| credentials, | ||
| )), | ||
| ) -> ChannelBuilder { | ||
| ChannelBuilder { | ||
| target: target.into(), | ||
| credentials: credentials, | ||
| channel_authority: None, | ||
| runtime: default_runtime(), | ||
| } | ||
| } | ||
|
|
||
| // TODO: enter_idle(&self) and graceful_stop()? | ||
|
|
||
| /// Returns the current state of the channel. If `connect` is true and the | ||
|
|
@@ -222,53 +153,88 @@ | |
| } | ||
| } | ||
|
|
||
| // A PersistentChannel represents the static configuration of a channel and an | ||
| // optional Arc of an ActiveChannel. An ActiveChannel exists whenever the | ||
| // PersistentChannel is not IDLE. Every channel is IDLE at creation, or after | ||
| // some configurable timeout elapses without any any RPC activity. | ||
| struct PersistentChannel { | ||
| target: Target, | ||
| resolver_builder: Arc<dyn ResolverBuilder>, | ||
| options: ChannelOptions, | ||
| active_channel: Mutex<Option<Arc<ActiveChannel>>>, | ||
| pub struct ChannelBuilder { | ||
| // Required values. | ||
| target: String, | ||
| credentials: Arc<dyn ChannelCredentials>, | ||
| runtime: GrpcRuntime, | ||
| security_opts: SecurityOpts, | ||
| authority: String, | ||
|
|
||
| // Optional values. | ||
| channel_authority: Option<String>, | ||
| // TODO(nathanielford) In follow-up implement the following optional values: | ||
| // - default_service_config | ||
| // - http_proxy_cfg | ||
| // - disable_health_checks | ||
| // - idle_timeout | ||
| // - enable_channelz | ||
| // - keepalive_cfg | ||
| } | ||
|
|
||
| impl PersistentChannel { | ||
| // Channels begin idle so `new()` does not automatically connect. | ||
| // ChannelOption contain only optional parameters. | ||
| fn new( | ||
| target: impl Into<String>, | ||
| runtime: GrpcRuntime, | ||
| options: ChannelOptions, | ||
| credentials: Arc<dyn ChannelCredentials>, | ||
| ) -> Self { | ||
| // TODO(nathanielford): Return errors here instead of panicking. | ||
| let target = Target::from_str(&target.into()).unwrap(); | ||
| impl ChannelBuilder { | ||
| /// Builds the channel with the provided configuration. | ||
| /// # Example | ||
| /// | ||
| /// ``` | ||
| /// use std::sync::Arc; | ||
| /// use grpc::client::Channel; | ||
| /// use grpc::credentials::LocalChannelCredentials; | ||
| /// | ||
| /// let channel = Channel::builder("dns:///localhost:123", Arc::new(LocalChannelCredentials::new())) | ||
| /// .build(); | ||
| /// ``` | ||
| pub fn build(self) -> Channel { | ||
| // TODO(nathanielford) This construction is currently a rough-cut placeholder. | ||
| // The design of PersistentChannel and how and where it is initialized | ||
| // will be finalized with the 'Internal Channel Design' with | ||
| // consideration for: | ||
| // - error handling (inc. always-failing resolvers due to invalid targets)) | ||
| // - testing (inc. credential and transport configuration) | ||
|
|
||
| // TODO(nathanielford) Find a better place to set up default registries. | ||
| setup_registeries(); | ||
|
Comment on lines
+193
to
+194
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's wrong with registering plugins here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be fine, and we might just leave it. However, this sets up the default registries for the entire library for all channels. It's a bit odd for this to be the responsibility of the channel builder, though it may be the first and only reasonable place for it to happen in actual usage. |
||
|
|
||
| let target = Target::from_str(self.target.as_str()).unwrap(); | ||
| let resolver_builder = global_registry().get(target.scheme()).unwrap(); | ||
|
nathanielford marked this conversation as resolved.
|
||
| let authority = options | ||
| // TODO(nathanielford): Return errors here instead of panicking. | ||
| let target = name_resolution::Target::from(target); | ||
| let authority = self | ||
| .channel_authority | ||
| .clone() | ||
| .unwrap_or_else(|| resolver_builder.default_authority(&target).to_owned()); | ||
| let security_opts = SecurityOpts { | ||
| credentials, | ||
| credentials: self.credentials, | ||
| authority: Authority::from_host_port_str(&authority), | ||
| handshake_info: ClientHandshakeInfo::default(), | ||
| }; | ||
|
|
||
| Self { | ||
| target, | ||
| resolver_builder, | ||
| active_channel: Mutex::default(), | ||
| options, | ||
| runtime, | ||
| security_opts, | ||
| authority, | ||
| Channel { | ||
| inner: Arc::new(PersistentChannel { | ||
| active_channel: Mutex::default(), | ||
| target, | ||
| security_opts, | ||
| runtime: self.runtime, | ||
| resolver_builder, | ||
| }), | ||
| } | ||
| } | ||
|
|
||
| pub fn channel_authority(mut self, authority: impl Into<String>) -> Self { | ||
| self.channel_authority = Some(authority.into()); | ||
| self | ||
| } | ||
| } | ||
|
|
||
| struct PersistentChannel { | ||
| active_channel: Mutex<Option<Arc<ActiveChannel>>>, | ||
|
|
||
| // Configuration | ||
| target: Target, | ||
| security_opts: SecurityOpts, | ||
| runtime: GrpcRuntime, | ||
|
|
||
| // Inferred Configuration | ||
| resolver_builder: Arc<dyn ResolverBuilder>, | ||
| } | ||
|
|
||
| impl PersistentChannel { | ||
| /// Returns the current state of the channel. If there is no underlying active channel, | ||
| /// returns Idle. If `connect` is true, will create a new active channel iff none exists. | ||
| fn get_state(&self, connect: bool) -> ConnectivityState { | ||
|
|
@@ -325,7 +291,8 @@ | |
|
|
||
| let work_scheduler = Arc::new(ResolverWorkScheduler { wqtx }); | ||
| let resolver_opts = name_resolution::ResolverOptions { | ||
| authority: persistent_channel.authority.clone(), | ||
| // authority: persistent_channel.security_opts.authority.clone(), | ||
| authority: "ignored".to_string(), // TODO(nathanielford) currently, this option is always ignored. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This authority will get used by the xDS resolver, we should keep it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to leave this for a follow up; the |
||
| work_scheduler, | ||
| runtime: runtime.clone(), | ||
| }; | ||
|
|
@@ -616,3 +583,17 @@ | |
| } | ||
| Authority::new(host_and_port.to_string(), None) | ||
| } | ||
|
|
||
| // Sets up the default registries for transports, name resolvers, and load | ||
| // balancers. | ||
| fn setup_registeries() { | ||
| pick_first::reg(); | ||
| round_robin::reg(); | ||
| dns::reg(); | ||
| #[cfg(unix)] | ||
| name_resolution::unix::reg(); | ||
| #[cfg(target_os = "linux")] | ||
| name_resolution::unix_abstract::reg(); | ||
| #[cfg(feature = "_runtime-tokio")] | ||
| tonic_transport::reg(); | ||
| } | ||
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.
Since many of these features are not yet implemented, adding a TODO to configure them feels premature. I recommend omitting the comment, as it lacks a tracking issue and is likely to become stale.
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.
Are we using tracking issues for any of this? I was told we weren't, really.
That said, I do intend to address all of these in near-future followups for optional values and for the ServiceConfig work (which is actively blocked on this). I'm not entirely sure the best way to track the state of work given the uncomfortable tension between a) avoiding TODOs, b) not using issues, c) avoiding huge CLs.