From 9d18a0821dc74957f0770653772a7fdd89a7b691 Mon Sep 17 00:00:00 2001 From: Milos Stankovic <82043364+morph-dev@users.noreply.github.com> Date: Sun, 9 Feb 2025 10:53:34 +0200 Subject: [PATCH] feat: remove LegacyHistory store version (#1674) --- .../src/versioned/id_indexed_v1/migration.rs | 179 ------------------ .../src/versioned/id_indexed_v1/mod.rs | 1 - .../src/versioned/id_indexed_v1/store.rs | 12 +- crates/storage/src/versioned/mod.rs | 7 +- crates/storage/src/versioned/utils.rs | 114 ++++------- 5 files changed, 45 insertions(+), 268 deletions(-) delete mode 100644 crates/storage/src/versioned/id_indexed_v1/migration.rs diff --git a/crates/storage/src/versioned/id_indexed_v1/migration.rs b/crates/storage/src/versioned/id_indexed_v1/migration.rs deleted file mode 100644 index bfdf538b6..000000000 --- a/crates/storage/src/versioned/id_indexed_v1/migration.rs +++ /dev/null @@ -1,179 +0,0 @@ -use tracing::{debug, info}; - -use super::IdIndexedV1StoreConfig; -use crate::{ - error::ContentStoreError, - versioned::{id_indexed_v1::sql, ContentType}, -}; - -pub fn migrate_legacy_history_store( - config: &IdIndexedV1StoreConfig, -) -> Result<(), ContentStoreError> { - if config.content_type != ContentType::History { - panic!("Can't migrate LegacyHistory store for non History content type.") - } - let content_type = &config.content_type; - - info!(content_type = %content_type, "Migration started"); - - let new_table_name = sql::table_name(content_type); - - // Rename table - debug!(content_type = %content_type, "Renaming table: history -> {new_table_name}"); - config.sql_connection_pool.get()?.execute_batch(&format!( - "ALTER TABLE history RENAME TO {};", - new_table_name - ))?; - - // Drop old indicies (they can't be renamed) - debug!(content_type = %content_type, "Dropping old indices"); - config.sql_connection_pool.get()?.execute_batch( - "DROP INDEX history_distance_short_idx; - DROP INDEX history_content_size_idx;", - )?; - - // Create new indicies - debug!(content_type = %content_type, "Creating new indices"); - config.sql_connection_pool.get()?.execute_batch(&format!( - "CREATE INDEX IF NOT EXISTS {0}_distance_short_idx ON {0} (distance_short); - CREATE INDEX IF NOT EXISTS {0}_content_size_idx ON {0} (content_size);", - new_table_name - ))?; - - info!(content_type = %content_type, "Migration finished"); - Ok(()) -} - -#[cfg(test)] -mod tests { - use std::collections::HashMap; - - use anyhow::Result; - use ethportal_api::{ - types::network::Subnetwork, IdentityContentKey, OverlayContentKey, RawContentValue, - }; - use rand::Rng; - - use super::*; - use crate::{ - test_utils::{create_test_portal_storage_config_with_capacity, generate_random_bytes}, - versioned::{usage_stats::UsageStats, IdIndexedV1Store, VersionedContentStore}, - }; - - const STORAGE_CAPACITY_MB: u32 = 10; - - mod legacy_history { - // Minimal code needed to test migration (since original code is deleted) - - use rusqlite::params; - - use super::*; - use crate::PortalStorageConfig; - - const CREATE_QUERY_DB_HISTORY: &str = "CREATE TABLE IF NOT EXISTS history ( - content_id blob PRIMARY KEY, - content_key blob NOT NULL, - content_value blob NOT NULL, - distance_short INTEGER NOT NULL, - content_size INTEGER NOT NULL - ); - CREATE INDEX IF NOT EXISTS history_distance_short_idx ON history(content_size); - CREATE INDEX IF NOT EXISTS history_content_size_idx ON history(distance_short); - "; - - const INSERT_QUERY_HISTORY: &str = - "INSERT OR IGNORE INTO history (content_id, content_key, content_value, distance_short, content_size) - VALUES (?1, ?2, ?3, ?4, ?5)"; - - pub fn create_store(config: &PortalStorageConfig) -> Result<()> { - config - .sql_connection_pool - .get()? - .execute_batch(CREATE_QUERY_DB_HISTORY)?; - Ok(()) - } - - pub fn store( - config: &PortalStorageConfig, - key: &impl OverlayContentKey, - value: &Vec, - ) -> Result<()> { - let content_id = key.content_id(); - let key = key.to_bytes(); - let distance_u32 = config - .distance_fn - .distance(&config.node_id, &content_id) - .big_endian_u32(); - let content_size = content_id.len() + key.len() + value.len(); - config.sql_connection_pool.get()?.execute( - INSERT_QUERY_HISTORY, - params![ - content_id.as_slice(), - key.to_vec(), - value, - distance_u32, - content_size - ], - )?; - Ok(()) - } - } - - fn generate_key_value_with_content_size() -> (IdentityContentKey, Vec) { - let key = IdentityContentKey::random(); - let value = generate_random_bytes(rand::thread_rng().gen_range(100..200)); - (key, value) - } - - #[test] - fn legacy_history_empty() -> Result<()> { - let (_temp_dir, config) = - create_test_portal_storage_config_with_capacity(STORAGE_CAPACITY_MB)?; - - // initialize legacy store - legacy_history::create_store(&config)?; - - // migrate - let config = IdIndexedV1StoreConfig::new(ContentType::History, Subnetwork::History, config); - migrate_legacy_history_store(&config)?; - - // make sure we can initialize new store and that it's empty - let store = IdIndexedV1Store::::create(ContentType::History, config)?; - assert_eq!(store.usage_stats(), UsageStats::default(),); - - Ok(()) - } - - #[test] - fn legacy_history_with_content() -> Result<()> { - let (_temp_dir, config) = - create_test_portal_storage_config_with_capacity(STORAGE_CAPACITY_MB)?; - - let mut key_value_map = HashMap::new(); - - // initialize legacy store - legacy_history::create_store(&config)?; - for _ in 0..10 { - let (key, value) = generate_key_value_with_content_size(); - legacy_history::store(&config, &key, &value)?; - key_value_map.insert(key, value); - } - - // migrate - let config = IdIndexedV1StoreConfig::new(ContentType::History, Subnetwork::History, config); - migrate_legacy_history_store(&config)?; - - // create IdIndexedV1Store and verify content - let store = IdIndexedV1Store::::create(ContentType::History, config)?; - for (key, value) in key_value_map.into_iter() { - assert_eq!( - store - .lookup_content_value(&key.content_id().into())? - .map(RawContentValue::into), - Some(value), - ); - } - - Ok(()) - } -} diff --git a/crates/storage/src/versioned/id_indexed_v1/mod.rs b/crates/storage/src/versioned/id_indexed_v1/mod.rs index d9cef0554..a771b00f1 100644 --- a/crates/storage/src/versioned/id_indexed_v1/mod.rs +++ b/crates/storage/src/versioned/id_indexed_v1/mod.rs @@ -1,5 +1,4 @@ mod config; -mod migration; mod pruning_strategy; pub(super) mod sql; mod store; diff --git a/crates/storage/src/versioned/id_indexed_v1/store.rs b/crates/storage/src/versioned/id_indexed_v1/store.rs index 8bc14b7ea..6032c8bae 100644 --- a/crates/storage/src/versioned/id_indexed_v1/store.rs +++ b/crates/storage/src/versioned/id_indexed_v1/store.rs @@ -7,10 +7,7 @@ use rusqlite::{named_params, types::Type, OptionalExtension}; use tracing::{debug, error, warn}; use trin_metrics::storage::StorageMetricsReporter; -use super::{ - migration::migrate_legacy_history_store, pruning_strategy::PruningStrategy, sql, - IdIndexedV1StoreConfig, -}; +use super::{pruning_strategy::PruningStrategy, sql, IdIndexedV1StoreConfig}; use crate::{ error::ContentStoreError, utils::get_total_size_of_directory_in_bytes, @@ -63,13 +60,10 @@ impl VersionedContentStore for IdIndexedV1Store< } fn migrate_from( - content_type: &ContentType, + _content_type: &ContentType, old_version: StoreVersion, - config: &Self::Config, + _config: &Self::Config, ) -> Result<(), ContentStoreError> { - if content_type == &ContentType::History && old_version == StoreVersion::LegacyHistory { - return migrate_legacy_history_store(config); - } Err(ContentStoreError::UnsupportedStoreMigration { old_version, new_version: Self::version(), diff --git a/crates/storage/src/versioned/mod.rs b/crates/storage/src/versioned/mod.rs index 7372521ac..7434269ca 100644 --- a/crates/storage/src/versioned/mod.rs +++ b/crates/storage/src/versioned/mod.rs @@ -32,12 +32,13 @@ pub enum ContentType { #[derive(Clone, Debug, Display, Eq, PartialEq, EnumString, AsRefStr)] #[strum(serialize_all = "snake_case")] pub enum StoreVersion { + /// The mock content store that can be used for tests. Implementation: + /// [utils::MockContentStore]. + MockContentStore, /// The store designed for content-id, content-key and content-value objects. /// The content from different subnetwork (expressed with `ContentType`) - /// uses different table. NOTE: implementation is in progress. + /// uses different table. Implementation: [IdIndexedV1Store]. IdIndexedV1, - /// The original SQLite version of the history storage store. - LegacyHistory, } impl FromSql for StoreVersion { diff --git a/crates/storage/src/versioned/utils.rs b/crates/storage/src/versioned/utils.rs index 502e1a005..61febbd0e 100644 --- a/crates/storage/src/versioned/utils.rs +++ b/crates/storage/src/versioned/utils.rs @@ -3,7 +3,7 @@ use r2d2_sqlite::SqliteConnectionManager; use rusqlite::{named_params, OptionalExtension}; use super::{sql, store::VersionedContentStore, ContentType, StoreVersion}; -use crate::error::ContentStoreError; +use crate::{error::ContentStoreError, PortalStorageConfig}; /// Ensures that the correct version of the content store is used (by migrating the content if /// that's not the case). @@ -41,30 +41,7 @@ fn get_store_version( |row| row.get::<&str, StoreVersion>("version"), ) .optional()?; - - match version { - Some(_) => Ok(version), - None => get_default_store_version(content_type, conn), - } -} - -fn get_default_store_version( - content_type: &ContentType, - conn: &PooledConnection, -) -> Result, ContentStoreError> { - match content_type { - ContentType::History => { - let exists = conn - .prepare(sql::TABLE_EXISTS)? - .exists(named_params! {":table_name": "history"})?; - if exists { - Ok(Some(StoreVersion::LegacyHistory)) - } else { - Ok(None) - } - } - _ => Ok(None), - } + Ok(version) } fn update_store_info( @@ -82,12 +59,41 @@ fn update_store_info( Ok(()) } +/// The mock content store, to be used in tests. +pub struct MockContentStore; + +impl VersionedContentStore for MockContentStore { + type Config = PortalStorageConfig; + + fn version() -> StoreVersion { + StoreVersion::MockContentStore + } + + fn migrate_from( + _content_type: &ContentType, + old_version: StoreVersion, + _config: &PortalStorageConfig, + ) -> Result<(), ContentStoreError> { + Err(ContentStoreError::UnsupportedStoreMigration { + old_version, + new_version: Self::version(), + }) + } + + fn create( + _content_type: ContentType, + _config: PortalStorageConfig, + ) -> Result { + Ok(Self {}) + } +} + #[cfg(test)] pub mod test { use anyhow::Result; use super::*; - use crate::{test_utils::create_test_portal_storage_config_with_capacity, PortalStorageConfig}; + use crate::test_utils::create_test_portal_storage_config_with_capacity; const STORAGE_CAPACITY_MB: u32 = 10; @@ -101,22 +107,6 @@ pub mod test { Ok(()) } - #[test] - fn get_store_version_default_history() -> Result<()> { - let (_temp_dir, config) = - create_test_portal_storage_config_with_capacity(STORAGE_CAPACITY_MB)?; - let conn = config.sql_connection_pool.get()?; - - let create_dummy_history_table_sql = "CREATE TABLE history (content_id blob PRIMARY KEY);"; - conn.execute(create_dummy_history_table_sql, [])?; - - assert_eq!( - get_store_version(&ContentType::History, &conn)?, - Some(StoreVersion::LegacyHistory) - ); - Ok(()) - } - #[test] fn insert_store_verion() -> Result<()> { let (_temp_dir, config) = @@ -139,10 +129,10 @@ pub mod test { let conn = config.sql_connection_pool.get()?; // Set store version - update_store_info(&ContentType::State, StoreVersion::LegacyHistory, &conn)?; + update_store_info(&ContentType::State, StoreVersion::MockContentStore, &conn)?; assert_eq!( get_store_version(&ContentType::State, &conn)?, - Some(StoreVersion::LegacyHistory) + Some(StoreVersion::MockContentStore) ); // Update store version @@ -170,7 +160,7 @@ pub mod test { assert_eq!( get_store_version(&ContentType::State, &sql_connection_pool.get()?)?, - Some(StoreVersion::IdIndexedV1) + Some(StoreVersion::MockContentStore) ); Ok(()) @@ -184,7 +174,7 @@ pub mod test { update_store_info( &ContentType::State, - StoreVersion::IdIndexedV1, + StoreVersion::MockContentStore, &sql_connection_pool.get()?, )?; @@ -193,7 +183,7 @@ pub mod test { assert_eq!( get_store_version(&ContentType::State, &config.sql_connection_pool.get()?)?, - Some(StoreVersion::IdIndexedV1) + Some(StoreVersion::MockContentStore) ); Ok(()) @@ -208,7 +198,7 @@ pub mod test { update_store_info( &ContentType::History, - StoreVersion::LegacyHistory, + StoreVersion::IdIndexedV1, &sql_connection_pool.get().unwrap(), ) .unwrap(); @@ -217,32 +207,4 @@ pub mod test { create_store::(ContentType::History, config, sql_connection_pool) .unwrap(); } - - pub struct MockContentStore; - - impl VersionedContentStore for MockContentStore { - type Config = PortalStorageConfig; - - fn version() -> StoreVersion { - StoreVersion::IdIndexedV1 - } - - fn migrate_from( - _content_type: &ContentType, - old_version: StoreVersion, - _config: &PortalStorageConfig, - ) -> Result<(), ContentStoreError> { - Err(ContentStoreError::UnsupportedStoreMigration { - old_version, - new_version: Self::version(), - }) - } - - fn create( - _content_type: ContentType, - _config: PortalStorageConfig, - ) -> Result { - Ok(Self {}) - } - } }