Skip to content
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

feat(config): Make gRPC http connections configurable #832

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

iamjpotts
Copy link
Contributor

@iamjpotts iamjpotts commented Sep 2, 2024

Description:
Make characteristics of the gRPC connections, such as keep alives and timeouts, configurable via ClientBuilder.

  • Add a new EndpointConfig struct with configuration values for the tonic Endpoint instances that will be created later
  • Add a new endpoint_config method on ClientBuilder to override the default configuration
  • Make ClientBuilder public
  • Add a new private ManagedNetworkBuilder enum which is accepted by ClientBuilder instead of ManagedNetwork - allowing construction of ManagedNetwork to be delayed until ClientBuilder::build is called, since ManagedNetwork requires EndpointConfig

All of the Client constructor methods now return Result<Self, _> instead of simply Self - this is due to the delayed parsing of the keys of HashMap<String, AccountId> into HostPort, and could be avoided if usage of HashMap<String, AccountId> were replaced with HashMap<HostPort, AccountId> (or HashMap<HostPort, EntityId>).

Related issue(s):

#833
#834

Notes for reviewer:

The node endpoint configuration capability is applicable to any combination of network nodes - both the preconfigured networks (mainnet, testnet, and previewnet), and custom networks. It would be possible, but inconvenient, to add a means to set endpoint configuration on either a Client instance or on one of the Client constructor methods.

Instead, this PR makes ClientBuilder public and adds a method to the builder to override the default endpoint configuration.

Tonic's Endpoint instances are setup when NodeConnection creates a channel:

pub(crate) fn channel(&self) -> Channel {
let channel = self
.channel
.get_or_init(|| {
let addresses = self.addresses.iter().map(|it| {
Endpoint::from_shared(format!("tcp://{it}"))
.unwrap()
.keep_alive_timeout(Duration::from_secs(10))
.keep_alive_while_idle(true)
.tcp_keepalive(Some(Duration::from_secs(10)))
.connect_timeout(Duration::from_secs(10))
});

Each NodeConnection is created in either NetworkData::from_addresses or NetworkData::with_address_book

Constructor call sites

NetworkData::from_addresses is typically called during initial creation of a Client via a constructor method on Client

Example:

pub fn for_network(network: HashMap<String, AccountId>) -> crate::Result<Self> {
let network =
ManagedNetwork::new(Network::from_addresses(&network)?, MirrorNetwork::default());
Ok(ClientBuilder::new(network).disable_network_updating().build())
}

Dynamic network update call sites

NetworkData::with_address_book is a part of the dynamic network update process where a client can periodically alter its preferred choice of nodes, though it can also be called via Client::set_network_from_address_book on a client instance.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@iamjpotts iamjpotts force-pushed the 20240901-endpoint-config branch 8 times, most recently from 61ba2b7 to ac877f1 Compare September 3, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants