From 800aabce2d2fd69e9fd7ab446216b559350b6e77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karol=20Bary=C5=82a?= Date: Sun, 29 Dec 2024 23:46:48 +0100 Subject: [PATCH] Move handling of MissingUserDefinedType to ClusterData::new This commit changes type of `keyspaces` field in `Metadata` from `HashMap` to `HashMap>`. Because of that, it also removed `MissingUserDefinedType` handling from `query_metadata`. Now handling this error is done in `ClusterData::new`. This has an advantage: we can use older version of the keyspace metadata if the new version has this error. --- scylla/src/cluster/metadata.rs | 15 +----- scylla/src/cluster/state.rs | 26 ++++++++-- scylla/src/cluster/worker.rs | 2 + scylla/src/policies/load_balancing/default.rs | 3 ++ scylla/src/routing/locator/mod.rs | 48 ++++++++++++++++--- .../routing/locator/precomputed_replicas.rs | 8 ++-- scylla/src/routing/locator/test.rs | 17 ++++--- 7 files changed, 86 insertions(+), 33 deletions(-) diff --git a/scylla/src/cluster/metadata.rs b/scylla/src/cluster/metadata.rs index 9c9f4f8b39..00359a3409 100644 --- a/scylla/src/cluster/metadata.rs +++ b/scylla/src/cluster/metadata.rs @@ -87,7 +87,7 @@ pub(crate) struct MetadataReader { /// Describes all metadata retrieved from the cluster pub(crate) struct Metadata { pub(crate) peers: Vec, - pub(crate) keyspaces: HashMap, + pub(crate) keyspaces: HashMap>, } #[non_exhaustive] // <- so that we can add more fields in a backwards-compatible way @@ -297,7 +297,7 @@ pub struct UserDefinedType { /// Represents a user defined type whose definition is missing from the metadata. #[derive(Clone, Debug, Error)] #[error("Missing UDT: {keyspace}, {name}")] -struct MissingUserDefinedType { +pub(crate) struct MissingUserDefinedType { name: String, keyspace: String, } @@ -800,17 +800,6 @@ async fn query_metadata( return Err(MetadataError::Peers(PeersMetadataError::EmptyTokenLists).into()); } - let keyspaces = keyspaces - .into_iter() - .filter_map(|(ks_name, ks)| match ks { - Ok(ks) => Some((ks_name, ks)), - Err(e) => { - warn!("Error while processing keyspace \"{ks_name}\": {e}"); - None - } - }) - .collect(); - Ok(Metadata { peers, keyspaces }) } diff --git a/scylla/src/cluster/state.rs b/scylla/src/cluster/state.rs index 9a7d891556..973d585b89 100644 --- a/scylla/src/cluster/state.rs +++ b/scylla/src/cluster/state.rs @@ -12,7 +12,7 @@ use scylla_cql::frame::response::result::TableSpec; use scylla_cql::types::serialize::row::SerializedValues; use std::collections::{HashMap, HashSet}; use std::sync::Arc; -use tracing::debug; +use tracing::{debug, warn}; use uuid::Uuid; use super::metadata::{Keyspace, Metadata, Strategy}; @@ -64,6 +64,7 @@ impl ClusterState { used_keyspace: &Option, host_filter: Option<&dyn HostFilter>, mut tablets: TabletsInfo, + old_keyspaces: &HashMap, ) -> Self { // Create new updated known_peers and ring let mut new_known_peers: HashMap> = @@ -109,6 +110,26 @@ impl ClusterState { } } + let keyspaces: HashMap = metadata + .keyspaces + .into_iter() + .filter_map(|(ks_name, ks)| match ks { + Ok(ks) => Some((ks_name, ks)), + Err(e) => { + if let Some(old_ks) = old_keyspaces.get(&ks_name) { + warn!("Encountered an error while processing metadata of keyspace \"{ks_name}\": {e}.\ + Re-using older version of this keyspace metadata"); + Some((ks_name, old_ks.clone())) + } else { + warn!("Encountered an error while processing metadata of keyspace \"{ks_name}\": {e}.\ + No previous version of this keyspace metadata found, so it will not be\ + present in ClusterData until next refresh."); + None + } + } + }) + .collect(); + { let removed_nodes = { let mut removed_nodes = HashSet::new(); @@ -122,7 +143,7 @@ impl ClusterState { }; let table_predicate = |spec: &TableSpec| { - if let Some(ks) = metadata.keyspaces.get(spec.ks_name()) { + if let Some(ks) = keyspaces.get(spec.ks_name()) { ks.tables.contains_key(spec.table_name()) } else { false @@ -150,7 +171,6 @@ impl ClusterState { ) } - let keyspaces = metadata.keyspaces; let (locator, keyspaces) = tokio::task::spawn_blocking(move || { let keyspace_strategies = keyspaces.values().map(|ks| &ks.strategy); let locator = ReplicaLocator::new(ring.into_iter(), keyspace_strategies, tablets); diff --git a/scylla/src/cluster/worker.rs b/scylla/src/cluster/worker.rs index 11ca6a8cad..3dab166fc5 100644 --- a/scylla/src/cluster/worker.rs +++ b/scylla/src/cluster/worker.rs @@ -140,6 +140,7 @@ impl Cluster { &None, host_filter.as_deref(), TabletsInfo::new(), + &HashMap::new(), ) .await; cluster_data.wait_until_all_pools_are_initialized().await; @@ -413,6 +414,7 @@ impl ClusterWorker { &self.used_keyspace, self.host_filter.as_deref(), cluster_data.locator.tablets.clone(), + &cluster_data.keyspaces, ) .await, ); diff --git a/scylla/src/policies/load_balancing/default.rs b/scylla/src/policies/load_balancing/default.rs index e15fa943e4..13a2540ebb 100644 --- a/scylla/src/policies/load_balancing/default.rs +++ b/scylla/src/policies/load_balancing/default.rs @@ -1419,6 +1419,7 @@ mod tests { &None, None, TabletsInfo::new(), + &HashMap::new(), ) .await } @@ -1449,6 +1450,7 @@ mod tests { &None, None, TabletsInfo::new(), + &HashMap::new(), ) .await } @@ -2498,6 +2500,7 @@ mod tests { Some(&FHostFilter) }, TabletsInfo::new(), + &HashMap::new(), ) .await; diff --git a/scylla/src/routing/locator/mod.rs b/scylla/src/routing/locator/mod.rs index 0ee5dfb8ef..f2d0dd47b9 100644 --- a/scylla/src/routing/locator/mod.rs +++ b/scylla/src/routing/locator/mod.rs @@ -860,21 +860,39 @@ mod tests { check( 160, None, - &metadata.keyspaces.get(KEYSPACE_NTS_RF_3).unwrap().strategy, + &metadata + .keyspaces + .get(KEYSPACE_NTS_RF_3) + .unwrap() + .as_ref() + .unwrap() + .strategy, TABLE_NTS_RF_3, vec![F, A, C, D, G, E], ); check( 160, None, - &metadata.keyspaces.get(KEYSPACE_NTS_RF_2).unwrap().strategy, + &metadata + .keyspaces + .get(KEYSPACE_NTS_RF_2) + .unwrap() + .as_ref() + .unwrap() + .strategy, TABLE_NTS_RF_2, vec![F, A, D, G], ); check( 160, None, - &metadata.keyspaces.get(KEYSPACE_SS_RF_2).unwrap().strategy, + &metadata + .keyspaces + .get(KEYSPACE_SS_RF_2) + .unwrap() + .as_ref() + .unwrap() + .strategy, TABLE_SS_RF_2, vec![F, A], ); @@ -882,21 +900,39 @@ mod tests { check( 160, Some("eu"), - &metadata.keyspaces.get(KEYSPACE_NTS_RF_3).unwrap().strategy, + &metadata + .keyspaces + .get(KEYSPACE_NTS_RF_3) + .unwrap() + .as_ref() + .unwrap() + .strategy, TABLE_NTS_RF_3, vec![A, C, G], ); check( 160, Some("us"), - &metadata.keyspaces.get(KEYSPACE_NTS_RF_3).unwrap().strategy, + &metadata + .keyspaces + .get(KEYSPACE_NTS_RF_3) + .unwrap() + .as_ref() + .unwrap() + .strategy, TABLE_NTS_RF_3, vec![F, D, E], ); check( 160, Some("eu"), - &metadata.keyspaces.get(KEYSPACE_SS_RF_2).unwrap().strategy, + &metadata + .keyspaces + .get(KEYSPACE_SS_RF_2) + .unwrap() + .as_ref() + .unwrap() + .strategy, TABLE_SS_RF_2, vec![A], ); diff --git a/scylla/src/routing/locator/precomputed_replicas.rs b/scylla/src/routing/locator/precomputed_replicas.rs index 4121b410ed..f37282c204 100644 --- a/scylla/src/routing/locator/precomputed_replicas.rs +++ b/scylla/src/routing/locator/precomputed_replicas.rs @@ -231,14 +231,14 @@ mod tests { let mut metadata = mock_metadata_for_token_aware_tests(); metadata.keyspaces = [( "SimpleStrategy{rf=2}".into(), - Keyspace { + Ok(Keyspace { strategy: Strategy::SimpleStrategy { replication_factor: 2, }, tables: HashMap::new(), views: HashMap::new(), user_defined_types: HashMap::new(), - }, + }), )] .iter() .cloned() @@ -251,7 +251,7 @@ mod tests { metadata .keyspaces .values() - .map(|keyspace| &keyspace.strategy), + .map(|keyspace| &keyspace.as_ref().unwrap().strategy), ); let check = |token, replication_factor, expected_node_ids| { @@ -293,7 +293,7 @@ mod tests { metadata .keyspaces .values() - .map(|keyspace| &keyspace.strategy), + .map(|keyspace| &keyspace.as_ref().unwrap().strategy), ); let check = |token, dc, replication_factor, expected_node_ids| { diff --git a/scylla/src/routing/locator/test.rs b/scylla/src/routing/locator/test.rs index 50084e6c59..e0205c101e 100644 --- a/scylla/src/routing/locator/test.rs +++ b/scylla/src/routing/locator/test.rs @@ -118,18 +118,18 @@ pub(crate) fn mock_metadata_for_token_aware_tests() -> Metadata { let keyspaces = [ ( KEYSPACE_SS_RF_2.into(), - Keyspace { + Ok(Keyspace { strategy: Strategy::SimpleStrategy { replication_factor: 2, }, tables: HashMap::new(), views: HashMap::new(), user_defined_types: HashMap::new(), - }, + }), ), ( KEYSPACE_NTS_RF_2.into(), - Keyspace { + Ok(Keyspace { strategy: Strategy::NetworkTopologyStrategy { datacenter_repfactors: [("eu".to_owned(), 2), ("us".to_owned(), 2)] .into_iter() @@ -138,11 +138,11 @@ pub(crate) fn mock_metadata_for_token_aware_tests() -> Metadata { tables: HashMap::new(), views: HashMap::new(), user_defined_types: HashMap::new(), - }, + }), ), ( KEYSPACE_NTS_RF_3.into(), - Keyspace { + Ok(Keyspace { strategy: Strategy::NetworkTopologyStrategy { datacenter_repfactors: [("eu".to_owned(), 3), ("us".to_owned(), 3)] .into_iter() @@ -151,7 +151,7 @@ pub(crate) fn mock_metadata_for_token_aware_tests() -> Metadata { tables: HashMap::new(), views: HashMap::new(), user_defined_types: HashMap::new(), - }, + }), ), ] .iter() @@ -199,7 +199,10 @@ pub(crate) fn create_ring(metadata: &Metadata) -> impl Iterator ReplicaLocator { let ring = create_ring(metadata); - let strategies = metadata.keyspaces.values().map(|ks| &ks.strategy); + let strategies = metadata + .keyspaces + .values() + .map(|ks| &ks.as_ref().unwrap().strategy); ReplicaLocator::new(ring, strategies, TabletsInfo::new()) }