From b6b313d448467b64c87bb89e427d10bedebe3ed4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 25 Oct 2024 15:02:32 +0200 Subject: [PATCH 1/4] node: remove needless clone in `resolve_contact_points` We can consume the `to_resolve` vector and move the datacenter string, instead of cloning. --- scylla/src/transport/node.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/scylla/src/transport/node.rs b/scylla/src/transport/node.rs index c7f438362..6b9882f4d 100644 --- a/scylla/src/transport/node.rs +++ b/scylla/src/transport/node.rs @@ -323,18 +323,20 @@ pub(crate) async fn resolve_contact_points( }) => to_resolve.push((hostname, Some(datacenter.clone()))), }; } - let resolve_futures = to_resolve.iter().map(|(hostname, datacenter)| async move { - match resolve_hostname(hostname).await { - Ok(address) => Some(ResolvedContactPoint { - address, - datacenter: datacenter.clone(), - }), - Err(e) => { - warn!("Hostname resolution failed for {}: {}", hostname, &e); - None + let resolve_futures = to_resolve + .into_iter() + .map(|(hostname, datacenter)| async move { + match resolve_hostname(hostname).await { + Ok(address) => Some(ResolvedContactPoint { + address, + datacenter, + }), + Err(e) => { + warn!("Hostname resolution failed for {}: {}", hostname, &e); + None + } } - } - }); + }); let resolved: Vec<_> = futures::future::join_all(resolve_futures).await; initial_peers.extend(resolved.into_iter().flatten()); From 104236b07bfb336688656b3a5c0fe05232070397 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 25 Oct 2024 15:31:53 +0200 Subject: [PATCH 2/4] node: don't allocate vector during hostname resolution In previous version, we would needlessly allocate vectors while having iterators over SocketAddrs. --- scylla/src/transport/node.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/scylla/src/transport/node.rs b/scylla/src/transport/node.rs index 6b9882f4d..e05ecd07c 100644 --- a/scylla/src/transport/node.rs +++ b/scylla/src/transport/node.rs @@ -271,10 +271,13 @@ pub(crate) struct ResolvedContactPoint { // It prefers to return IPv4s first, and only if there are none, IPv6s. pub(crate) async fn resolve_hostname(hostname: &str) -> Result { let mut ret = None; - let addrs: Vec = match lookup_host(hostname).await { - Ok(addrs) => addrs.collect(), + let addrs = match lookup_host(hostname).await { + Ok(addrs) => itertools::Either::Left(addrs), // Use a default port in case of error, but propagate the original error on failure - Err(e) => lookup_host((hostname, 9042)).await.or(Err(e))?.collect(), + Err(e) => { + let addrs = lookup_host((hostname, 9042)).await.or(Err(e))?; + itertools::Either::Right(addrs) + } }; for a in addrs { match a { From 113f5003366b4e89d5134fbc8b64e9845886f5d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 25 Oct 2024 20:02:16 +0200 Subject: [PATCH 3/4] node: adjust `resolve_hostname` to functional programming Notice that using `find_or_last` is equivalent to previous version. --- scylla/src/transport/node.rs | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/scylla/src/transport/node.rs b/scylla/src/transport/node.rs index e05ecd07c..ba2b3d9f4 100644 --- a/scylla/src/transport/node.rs +++ b/scylla/src/transport/node.rs @@ -1,3 +1,4 @@ +use itertools::Itertools; use tokio::net::lookup_host; use tracing::warn; use uuid::Uuid; @@ -270,7 +271,6 @@ pub(crate) struct ResolvedContactPoint { // The resolution may return multiple IPs and the function returns one of them. // It prefers to return IPv4s first, and only if there are none, IPv6s. pub(crate) async fn resolve_hostname(hostname: &str) -> Result { - let mut ret = None; let addrs = match lookup_host(hostname).await { Ok(addrs) => itertools::Either::Left(addrs), // Use a default port in case of error, but propagate the original error on failure @@ -279,21 +279,15 @@ pub(crate) async fn resolve_hostname(hostname: &str) -> Result return Ok(a), - _ => { - ret = Some(a); - } - } - } - ret.ok_or_else(|| { - io::Error::new( - io::ErrorKind::Other, - format!("Empty address list returned by DNS for {}", hostname), - ) - }) + addrs + .find_or_last(|addr| matches!(addr, SocketAddr::V4(_))) + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::Other, + format!("Empty address list returned by DNS for {}", hostname), + ) + }) } /// Transforms the given [`InternalKnownNode`]s into [`ContactPoint`]s. From 81d9576b186c84e998c9de1214826cf51b9719a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Fri, 25 Oct 2024 18:20:50 +0200 Subject: [PATCH 4/4] refiller: remove needless `has_connections()` method This is just a negation of `is_empty()` method. For reference: ``` fn is_empty(&self) -> bool { self.conns.iter().all(|conns| conns.is_empty()) } ``` --- scylla/src/transport/connection_pool.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/scylla/src/transport/connection_pool.rs b/scylla/src/transport/connection_pool.rs index 9e7a6b575..54d70c0eb 100644 --- a/scylla/src/transport/connection_pool.rs +++ b/scylla/src/transport/connection_pool.rs @@ -988,7 +988,7 @@ impl PoolRefiller { // `last_error` must not be `None` if there is a possibility of the pool // being empty. fn update_shared_conns(&mut self, last_error: Option) { - let new_conns = if !self.has_connections() { + let new_conns = if self.is_empty() { Arc::new(MaybePoolConnections::Broken(last_error.unwrap())) } else { let new_conns = if let Some(sharder) = self.sharder.as_ref() { @@ -1046,7 +1046,7 @@ impl PoolRefiller { self.conns[shard_id].len(), self.active_connection_count(), ); - if !self.has_connections() { + if self.is_empty() { let _ = self.pool_empty_notifier.send(()); } self.update_shared_conns(Some(last_error)); @@ -1152,10 +1152,6 @@ impl PoolRefiller { ); } - fn has_connections(&self) -> bool { - self.conns.iter().any(|v| !v.is_empty()) - } - fn active_connection_count(&self) -> usize { self.conns.iter().map(Vec::len).sum::() }