From c00cd95f9da79bc482d65551b96d7ffa0ab08491 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 11 Sep 2023 20:51:03 -0400 Subject: [PATCH] refactor: Extract a function for constructing NamespaceCreated messages in the router So that the same conversion can happen in the tests and one assert_eq! can check everything rather than repeating lots of assertions for every test for every field. --- router/src/gossip/mod.rs | 18 ++++++++ router/src/gossip/namespace_cache.rs | 37 ++++++---------- router/src/gossip/schema_change_observer.rs | 48 +++------------------ 3 files changed, 39 insertions(+), 64 deletions(-) diff --git a/router/src/gossip/mod.rs b/router/src/gossip/mod.rs index a6133050a3..7bafec94ba 100644 --- a/router/src/gossip/mod.rs +++ b/router/src/gossip/mod.rs @@ -52,6 +52,24 @@ pub mod namespace_cache; pub mod schema_change_observer; pub mod traits; +use data_types::NamespaceSchema; +use generated_types::influxdata::iox::gossip::v1::NamespaceCreated; + +/// Make a `NamespaceCreated` protobuf instance from the specified name and schema. +pub(crate) fn namespace_created( + namespace_name: impl Into, + schema: &NamespaceSchema, +) -> NamespaceCreated { + NamespaceCreated { + namespace_name: namespace_name.into(), + namespace_id: schema.id.get(), + partition_template: schema.partition_template.as_proto().cloned(), + max_columns_per_table: schema.max_columns_per_table as u64, + max_tables: schema.max_tables as u64, + retention_period_ns: schema.retention_period_ns, + } +} + #[cfg(test)] mod mock_schema_broadcast; diff --git a/router/src/gossip/namespace_cache.rs b/router/src/gossip/namespace_cache.rs index 01d77948a5..5fcdc9e7c5 100644 --- a/router/src/gossip/namespace_cache.rs +++ b/router/src/gossip/namespace_cache.rs @@ -396,7 +396,10 @@ mod tests { }; use crate::{ - gossip::tests::{DEFAULT_NAMESPACE, DEFAULT_NAMESPACE_PARTITION_TEMPLATE, NAMESPACE_NAME}, + gossip::{ + namespace_created, + tests::{DEFAULT_NAMESPACE, DEFAULT_NAMESPACE_PARTITION_TEMPLATE, NAMESPACE_NAME}, + }, namespace_cache::{CacheMissErr, MemoryNamespaceCache}, }; @@ -846,14 +849,10 @@ mod tests { test_handle_gossip_message_!( namespace_created_missing_name, existing = None, - message = Event::NamespaceCreated(NamespaceCreated { - namespace_name: "".to_string(), // missing in proto - namespace_id: DEFAULT_NAMESPACE.id.get(), - partition_template: Some((**PARTITION_BY_DAY_PROTO).clone()), - max_columns_per_table: DEFAULT_NAMESPACE.max_columns_per_table as _, - max_tables: DEFAULT_NAMESPACE.max_tables as _, - retention_period_ns: DEFAULT_NAMESPACE.retention_period_ns, - }), + message = Event::NamespaceCreated(namespace_created( + "", // missing name in proto + &DEFAULT_NAMESPACE, + )), want = Err(CacheMissErr { .. }) ); @@ -862,12 +861,8 @@ mod tests { namespace_created, existing = None, message = Event::NamespaceCreated(NamespaceCreated { - namespace_name: NAMESPACE_NAME.to_string(), - namespace_id: DEFAULT_NAMESPACE.id.get(), partition_template: None, - max_columns_per_table: DEFAULT_NAMESPACE.max_columns_per_table as _, - max_tables: DEFAULT_NAMESPACE.max_tables as _, - retention_period_ns: DEFAULT_NAMESPACE.retention_period_ns, + ..namespace_created(NAMESPACE_NAME, &DEFAULT_NAMESPACE) }), want = Ok(v) => { assert_eq!(*v, DEFAULT_NAMESPACE); @@ -880,12 +875,11 @@ mod tests { namespace_created_specified_partition_template, existing = None, message = Event::NamespaceCreated(NamespaceCreated { - namespace_name: NAMESPACE_NAME.to_string(), // missing in proto - namespace_id: DEFAULT_NAMESPACE.id.get(), partition_template: Some((**PARTITION_BY_DAY_PROTO).clone()), - max_columns_per_table: DEFAULT_NAMESPACE.max_columns_per_table as _, - max_tables: DEFAULT_NAMESPACE.max_tables as _, - retention_period_ns: DEFAULT_NAMESPACE.retention_period_ns, + ..namespace_created( + NAMESPACE_NAME, + &DEFAULT_NAMESPACE, + ) }), want = Ok(v) => { let mut want = DEFAULT_NAMESPACE.clone(); @@ -899,19 +893,16 @@ mod tests { namespace_created_existing, existing = Some(DEFAULT_NAMESPACE), message = Event::NamespaceCreated(NamespaceCreated { - namespace_name: NAMESPACE_NAME.to_string(), // missing in proto - namespace_id: DEFAULT_NAMESPACE.id.get(), - // The partition template is not allowed to change over the lifetime // of the namespace. partition_template: DEFAULT_NAMESPACE.partition_template.as_proto().cloned(), - // But these fields can change. // // They will be ignored, and the local values used instead. max_columns_per_table: 123456, max_tables: 123456, retention_period_ns: Some(123456), + ..namespace_created(NAMESPACE_NAME, &DEFAULT_NAMESPACE) }), want = Ok(v) => { // Mutable values remain unmodified diff --git a/router/src/gossip/schema_change_observer.rs b/router/src/gossip/schema_change_observer.rs index 75fcda47ae..ce62912378 100644 --- a/router/src/gossip/schema_change_observer.rs +++ b/router/src/gossip/schema_change_observer.rs @@ -5,12 +5,12 @@ use std::{collections::BTreeMap, fmt::Debug, sync::Arc}; use async_trait::async_trait; use data_types::{ColumnsByName, NamespaceName, NamespaceSchema}; use generated_types::influxdata::iox::gossip::v1::{ - schema_message::Event, Column, NamespaceCreated, TableCreated, TableUpdated, + schema_message::Event, Column, TableCreated, TableUpdated, }; use crate::namespace_cache::{ChangeStats, NamespaceCache}; -use super::traits::SchemaBroadcast; +use super::{namespace_created, traits::SchemaBroadcast}; /// A [`NamespaceCache`] decorator implementing cluster-wide, best-effort /// propagation of local schema changes via the gossip subsystem. @@ -116,14 +116,7 @@ where namespace_name: &NamespaceName<'static>, schema: &NamespaceSchema, ) { - let msg = NamespaceCreated { - namespace_name: namespace_name.to_string(), - namespace_id: schema.id.get(), - partition_template: schema.partition_template.as_proto().cloned(), - max_columns_per_table: schema.max_columns_per_table as u64, - max_tables: schema.max_tables as u64, - retention_period_ns: schema.retention_period_ns, - }; + let msg = namespace_created(namespace_name, schema); self.tx.broadcast(Event::NamespaceCreated(msg)); } @@ -262,20 +255,8 @@ mod tests { }, schema = DEFAULT_NAMESPACE, want_count = 1, - want = [Event::NamespaceCreated(NamespaceCreated { - namespace_name, - namespace_id, - partition_template, - max_columns_per_table, - max_tables, - retention_period_ns - })] => { - assert_eq!(namespace_name, NAMESPACE_NAME); - assert_eq!(*namespace_id, DEFAULT_NAMESPACE.id.get()); - assert_eq!(*partition_template, DEFAULT_NAMESPACE.partition_template.as_proto().cloned()); - assert_eq!(*max_columns_per_table, DEFAULT_NAMESPACE.max_columns_per_table as u64); - assert_eq!(*max_tables, DEFAULT_NAMESPACE.max_tables as u64); - assert_eq!(*retention_period_ns, DEFAULT_NAMESPACE.retention_period_ns); + want = [Event::NamespaceCreated(created)] => { + assert_eq!(created, &namespace_created(NAMESPACE_NAME, &DEFAULT_NAMESPACE)); } ); @@ -306,28 +287,13 @@ mod tests { }, schema = DEFAULT_NAMESPACE, want_count = 1, - want = [Event::NamespaceCreated(NamespaceCreated { - namespace_name, - namespace_id, - partition_template, - max_columns_per_table, - max_tables, - retention_period_ns - }), + want = [Event::NamespaceCreated(created), Event::TableCreated(TableCreated { table, partition_template: table_template })] => { // Validate the namespace create message - - assert_eq!(namespace_name, NAMESPACE_NAME); - assert_eq!(*namespace_id, DEFAULT_NAMESPACE.id.get()); - assert_eq!(*partition_template, DEFAULT_NAMESPACE.partition_template.as_proto().cloned()); - assert_eq!(*max_columns_per_table, DEFAULT_NAMESPACE.max_columns_per_table as u64); - assert_eq!(*max_tables, DEFAULT_NAMESPACE.max_tables as u64); - assert_eq!(*retention_period_ns, DEFAULT_NAMESPACE.retention_period_ns); + assert_eq!(created, &namespace_created(NAMESPACE_NAME, &DEFAULT_NAMESPACE)); // Validate the table message - let meta = table.as_ref().expect("must have metadata"); - assert_eq!(meta.table_name, TABLE_NAME); assert_eq!(meta.namespace_name, NAMESPACE_NAME); assert_eq!(meta.table_id, TABLE_ID);