From 18099c6ecd3b2d13d505f3af18f1413085b8a616 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 15 Jan 2025 20:29:56 +0100 Subject: [PATCH 1/6] codewide: remove needless `format!`s In multiple places in the code, `format!` is called only to subsequently pass the allocated String to a printing macro. In all such places, `format!` could be trivially elided. --- scylla/src/client/pager.rs | 2 +- scylla/src/client/session.rs | 2 +- scylla/src/cluster/metadata.rs | 6 ++---- scylla/src/network/connection_pool.rs | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/scylla/src/client/pager.rs b/scylla/src/client/pager.rs index 039d747590..797312d246 100644 --- a/scylla/src/client/pager.rs +++ b/scylla/src/client/pager.rs @@ -232,7 +232,7 @@ where let retry_decision = self.retry_session.decide_should_retry(query_info); trace!( parent: &span, - retry_decision = format!("{:?}", retry_decision).as_str() + retry_decision = ?retry_decision ); last_error = request_error.into_query_error(); diff --git a/scylla/src/client/session.rs b/scylla/src/client/session.rs index 2ba11889b7..dcc2d1d158 100644 --- a/scylla/src/client/session.rs +++ b/scylla/src/client/session.rs @@ -2096,7 +2096,7 @@ where let retry_decision = context.retry_session.decide_should_retry(query_info); trace!( parent: &span, - retry_decision = format!("{:?}", retry_decision).as_str() + retry_decision = ?retry_decision ); last_error = Some(request_error.into_query_error()); diff --git a/scylla/src/cluster/metadata.rs b/scylla/src/cluster/metadata.rs index 31657b33be..21325ec493 100644 --- a/scylla/src/cluster/metadata.rs +++ b/scylla/src/cluster/metadata.rs @@ -633,11 +633,9 @@ impl MetadataReader { }; warn!( - control_connection_address = self + control_connection_address = tracing::field::display(self .control_connection_endpoint - .address() - .to_string() - .as_str(), + .address()), error = %err, "Failed to fetch metadata using current control connection" ); diff --git a/scylla/src/network/connection_pool.rs b/scylla/src/network/connection_pool.rs index a750887e4a..4b99d868a9 100644 --- a/scylla/src/network/connection_pool.rs +++ b/scylla/src/network/connection_pool.rs @@ -587,7 +587,7 @@ impl PoolRefiller { } } trace!( - pool_state = format!("{:?}", ShardedConnectionVectorWrapper(&self.conns)).as_str() + pool_state = ?ShardedConnectionVectorWrapper(&self.conns) ); // Schedule refilling here From 9b1ac44857da33ecf9934249dd384da6f4035727 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 15 Jan 2025 20:33:38 +0100 Subject: [PATCH 2/6] codewide: employ CommaSeparatedDisplayer to avoid allocations Instead of allocating a `Vec<_>`, one can leverage `CommaSeparatedDisplayer` to print a series of items in the comma-separated manner. --- scylla/src/cluster/metadata.rs | 22 ++++++++-------------- scylla/src/network/connection_pool.rs | 10 ++++------ scylla/src/utils/pretty.rs | 7 +++++++ 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/scylla/src/cluster/metadata.rs b/scylla/src/cluster/metadata.rs index 21325ec493..f68cb43170 100644 --- a/scylla/src/cluster/metadata.rs +++ b/scylla/src/cluster/metadata.rs @@ -26,6 +26,7 @@ use crate::policies::host_filter::HostFilter; use crate::routing::Token; use crate::statement::query::Query; use crate::utils::parse::{ParseErrorCause, ParseResult, ParserState}; +use crate::utils::pretty::{CommaSeparatedDisplayer, DisplayUsingDebug}; use futures::future::{self, FutureExt}; use futures::stream::{self, StreamExt, TryStreamExt}; @@ -37,8 +38,7 @@ use scylla_macros::DeserializeRow; use std::borrow::BorrowMut; use std::cell::Cell; use std::collections::HashMap; -use std::fmt; -use std::fmt::Formatter; +use std::fmt::{self, Formatter}; use std::net::{IpAddr, SocketAddr}; use std::num::NonZeroUsize; use std::str::FromStr; @@ -562,11 +562,7 @@ impl MetadataReader { self.known_peers.shuffle(&mut thread_rng()); debug!( "Known peers: {}", - self.known_peers - .iter() - .map(|endpoint| format!("{:?}", endpoint)) - .collect::>() - .join(", ") + CommaSeparatedDisplayer(self.known_peers.iter().map(DisplayUsingDebug)) ); let address_of_failed_control_connection = self.control_connection_endpoint.address(); @@ -699,11 +695,9 @@ impl MetadataReader { // and print an error message about this fact if !metadata.peers.is_empty() && self.known_peers.is_empty() { error!( - node_ips = ?metadata - .peers - .iter() - .map(|peer| peer.address) - .collect::>(), + node_ips = tracing::field::display(CommaSeparatedDisplayer( + metadata.peers.iter().map(|peer| peer.address) + )), "The host filter rejected all nodes in the cluster, \ no connections that can serve user queries have been \ established. The session cannot serve any queries!" @@ -719,12 +713,12 @@ impl MetadataReader { if let Some(peer) = control_connection_peer { if !self.host_filter.as_ref().map_or(true, |f| f.accept(peer)) { warn!( - filtered_node_ips = ?metadata + filtered_node_ips = tracing::field::display(CommaSeparatedDisplayer(metadata .peers .iter() .filter(|peer| self.host_filter.as_ref().map_or(true, |p| p.accept(peer))) .map(|peer| peer.address) - .collect::>(), + )), control_connection_address = ?self.control_connection_endpoint.address(), "The node that the control connection is established to \ is not accepted by the host filter. Please verify that \ diff --git a/scylla/src/network/connection_pool.rs b/scylla/src/network/connection_pool.rs index 4b99d868a9..2390365f27 100644 --- a/scylla/src/network/connection_pool.rs +++ b/scylla/src/network/connection_pool.rs @@ -1,5 +1,6 @@ #[cfg(feature = "cloud")] use crate::cloud::set_ssl_config_for_scylla_cloud_host; +use crate::utils::pretty::CommaSeparatedDisplayer; use super::connection::{ open_connection, open_connection_to_shard_aware_port, Connection, ConnectionConfig, @@ -358,12 +359,9 @@ impl NodeConnectionPool { fn choose_random_connection_from_slice(v: &[Arc]) -> Option> { trace!( - connections = v - .iter() - .map(|conn| conn.get_connect_address().to_string()) - .collect::>() - .join(",") - .as_str(), + connections = tracing::field::display(CommaSeparatedDisplayer( + v.iter().map(|conn| conn.get_connect_address()) + )), "Available" ); if v.is_empty() { diff --git a/scylla/src/utils/pretty.rs b/scylla/src/utils/pretty.rs index 63ed28b42b..a0fb14b6d4 100644 --- a/scylla/src/utils/pretty.rs +++ b/scylla/src/utils/pretty.rs @@ -222,6 +222,13 @@ where } } +pub(crate) struct DisplayUsingDebug(pub(crate) T); +impl std::fmt::Display for DisplayUsingDebug { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + ::fmt(&self.0, f) + } +} + #[cfg(test)] mod tests { use chrono::{NaiveDate, NaiveDateTime, NaiveTime}; From b648157c2058fe034492212010158b3110817e46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 15 Jan 2025 20:41:49 +0100 Subject: [PATCH 3/6] driver_tracing: optimise record_replicas RequestSpan::record_replicas now: 1. accepts an iterator, not necessarily a slice, 2. utilises CommaSeparatedDisplayer instead of hand-crafted comma management. In the next commit, the point 1. is taken advantage of. --- scylla/src/client/pager.rs | 2 +- scylla/src/client/session.rs | 2 +- scylla/src/observability/driver_tracing.rs | 28 +++++++++++----------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/scylla/src/client/pager.rs b/scylla/src/client/pager.rs index 797312d246..73e6a23f5a 100644 --- a/scylla/src/client/pager.rs +++ b/scylla/src/client/pager.rs @@ -856,7 +856,7 @@ impl QueryPager { serialized_values_size, ); if let Some(replicas) = replicas.as_ref() { - span.record_replicas(replicas); + span.record_replicas(replicas.iter().map(|(node, shard)| (node, *shard))); } span }; diff --git a/scylla/src/client/session.rs b/scylla/src/client/session.rs index dcc2d1d158..5fb35b93fb 100644 --- a/scylla/src/client/session.rs +++ b/scylla/src/client/session.rs @@ -1429,7 +1429,7 @@ where let replicas: smallvec::SmallVec<[_; 8]> = cluster_data .get_token_endpoints_iter(table_spec, token) .collect(); - span.record_replicas(&replicas) + span.record_replicas(replicas.iter().map(|&(node, shard)| (node, shard))) } } diff --git a/scylla/src/observability/driver_tracing.rs b/scylla/src/observability/driver_tracing.rs index 5d4877c9f3..2fd97180af 100644 --- a/scylla/src/observability/driver_tracing.rs +++ b/scylla/src/observability/driver_tracing.rs @@ -118,26 +118,26 @@ impl RequestSpan { } } - pub(crate) fn record_replicas<'a>(&'a self, replicas: &'a [(impl Borrow>, Shard)]) { - struct ReplicaIps<'a, N>(&'a [(N, Shard)]); - impl Display for ReplicaIps<'_, N> + pub(crate) fn record_replicas<'a>( + &'a self, + replicas: impl Iterator> + 'a, Shard)> + Clone, + ) { + struct Replica(N, Shard); + impl Display for Replica where N: Borrow>, { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let mut nodes_with_shards = self.0.iter(); - if let Some((node, shard)) = nodes_with_shards.next() { - write!(f, "{}-shard{}", node.borrow().address.ip(), shard)?; - - for (node, shard) in nodes_with_shards { - write!(f, ",{}-shard{}", node.borrow().address.ip(), shard)?; - } - } - Ok(()) + let Self(node, shard) = self; + write!(f, "{}-shard{}", node.borrow().address.ip(), shard) } } - self.span - .record("replicas", tracing::field::display(&ReplicaIps(replicas))); + self.span.record( + "replicas", + tracing::field::display(CommaSeparatedDisplayer( + replicas.map(|(node, shard)| Replica(node, shard)), + )), + ); } pub(crate) fn record_request_size(&self, size: usize) { From 936e8ebcd6d49f78b7626b3277040a8ee022bf95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 15 Jan 2025 20:49:53 +0100 Subject: [PATCH 4/6] session: do not allocate replicas for tracing purposes Even though the replicas were allocated in a SmallVec, with the enhanced RequestSpan::record_replicas implementation we can elide the call to `collect()`. In order to make this possible, several iterators in the replica locator have now `Clone` derived. --- scylla/src/client/session.rs | 6 ++---- scylla/src/cluster/state.rs | 2 +- scylla/src/routing/locator/mod.rs | 2 ++ scylla/src/routing/locator/replicas.rs | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/scylla/src/client/session.rs b/scylla/src/client/session.rs index 5fb35b93fb..a23779a26e 100644 --- a/scylla/src/client/session.rs +++ b/scylla/src/client/session.rs @@ -1426,10 +1426,8 @@ where if !span.span().is_disabled() { if let (Some(table_spec), Some(token)) = (statement_info.table, token) { let cluster_data = self.get_cluster_data(); - let replicas: smallvec::SmallVec<[_; 8]> = cluster_data - .get_token_endpoints_iter(table_spec, token) - .collect(); - span.record_replicas(replicas.iter().map(|&(node, shard)| (node, shard))) + let replicas = cluster_data.get_token_endpoints_iter(table_spec, token); + span.record_replicas(replicas) } } diff --git a/scylla/src/cluster/state.rs b/scylla/src/cluster/state.rs index 97b75847b1..9a7d891556 100644 --- a/scylla/src/cluster/state.rs +++ b/scylla/src/cluster/state.rs @@ -217,7 +217,7 @@ impl ClusterState { &self, table_spec: &TableSpec, token: Token, - ) -> impl Iterator, Shard)> { + ) -> impl Iterator, Shard)> + Clone { let keyspace = self.keyspaces.get(table_spec.ks_name()); let strategy = keyspace .map(|k| &k.strategy) diff --git a/scylla/src/routing/locator/mod.rs b/scylla/src/routing/locator/mod.rs index f33b99fa3a..0ee5dfb8ef 100644 --- a/scylla/src/routing/locator/mod.rs +++ b/scylla/src/routing/locator/mod.rs @@ -472,6 +472,7 @@ impl<'a> IntoIterator for ReplicaSet<'a> { } } +#[derive(Clone)] enum ReplicaSetIteratorInner<'a> { /// Token ring with SimpleStrategy, any datacenter Plain { @@ -502,6 +503,7 @@ enum ReplicaSetIteratorInner<'a> { } /// Iterator that returns replicas from some replica set. +#[derive(Clone)] pub struct ReplicaSetIterator<'a> { inner: ReplicaSetIteratorInner<'a>, token: Token, diff --git a/scylla/src/routing/locator/replicas.rs b/scylla/src/routing/locator/replicas.rs index a07e8104b2..8a70a4c194 100644 --- a/scylla/src/routing/locator/replicas.rs +++ b/scylla/src/routing/locator/replicas.rs @@ -7,7 +7,7 @@ use std::{ops::Index, sync::Arc}; /// /// This type is very similar to `Cow`, but unlike `Cow`, /// it holds references in an `Owned` variant `Vec`. -#[derive(Debug)] +#[derive(Debug, Clone)] pub(crate) enum ReplicasArray<'a> { Borrowed(&'a [Arc]), Owned(Vec>), From 369226c01485db067e841b58b7f0b51b69b5a377 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 15 Jan 2025 20:52:55 +0100 Subject: [PATCH 5/6] metadata: elide peers allocation into Vec Perhaps the compiler would optimise this anyway, but now it's explicitly better. --- scylla/src/cluster/metadata.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scylla/src/cluster/metadata.rs b/scylla/src/cluster/metadata.rs index f68cb43170..922c4170f9 100644 --- a/scylla/src/cluster/metadata.rs +++ b/scylla/src/cluster/metadata.rs @@ -876,9 +876,10 @@ async fn query_peers(conn: &Arc, connect_port: u16) -> Result>() .await?; - Ok(peers.into_iter().flatten().collect()) + Ok(peers) } async fn create_peer_from_row( From a42992f0b790caa11d658827e2009920c9facff1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wojciech=20Przytu=C5=82a?= Date: Wed, 15 Jan 2025 20:53:56 +0100 Subject: [PATCH 6/6] locator: clone `Arc`s only after filtering duplicates This is a tiny optimisation. --- scylla/src/routing/locator/replication_info.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/scylla/src/routing/locator/replication_info.rs b/scylla/src/routing/locator/replication_info.rs index 16318dfd6d..7cae0ec558 100644 --- a/scylla/src/routing/locator/replication_info.rs +++ b/scylla/src/routing/locator/replication_info.rs @@ -64,8 +64,9 @@ impl ReplicationInfo { let unique_nodes_in_global_ring = global_ring .iter() - .map(|(_t, n)| n.clone()) + .map(|(_t, n)| n) .unique() + .cloned() .collect(); let mut datacenter_nodes: HashMap<&str, Vec<(Token, Arc)>> = HashMap::new(); @@ -82,7 +83,7 @@ impl ReplicationInfo { for (datacenter_name, this_datacenter_nodes) in datacenter_nodes { let dc_ring = TokenRing::new(this_datacenter_nodes.into_iter()); let unique_nodes_in_dc_ring = - dc_ring.iter().map(|(_t, n)| n.clone()).unique().collect(); + dc_ring.iter().map(|(_t, n)| n).unique().cloned().collect(); // When counting racks consider None as a separate rack let rack_count: usize = dc_ring .iter()