From f98d40658031f4c7a81aa48506faeac4de90bd4c Mon Sep 17 00:00:00 2001 From: evenyag Date: Mon, 8 Aug 2022 16:46:51 +0800 Subject: [PATCH] refactor(storage): Add region id and name to metadata (#140) * refactor(storage): Add region id and name to metadata Add region id and name to `RegionMetadata`, simplify input arguments of `RegionImpl::create()` and `RegionImpl::new()` method, since id and name are already in metadata/version. To avoid an atomic load of `Version` each time we access the region id/name, we still store a copy of id/name in `SharedData`. * chore: Remove todo in OpenOptions Create region if missing when opening the region would be hard to implement, since sometimes we may don't known the exact region schema user would like to have. * refactor: Make id and name of region readonly By making `id` and `name` fields of `SharedData` and `RegionMetadata` private and only exposing a pub getter. --- src/storage/src/engine.rs | 9 +------ src/storage/src/flush.rs | 4 +-- src/storage/src/metadata.rs | 35 ++++++++++++++++++++++++-- src/storage/src/region.rs | 36 ++++++++++++++++----------- src/storage/src/region/tests.rs | 7 +----- src/storage/src/region/tests/basic.rs | 4 +-- src/storage/src/region/tests/flush.rs | 4 +-- src/store-api/src/storage/engine.rs | 4 +-- 8 files changed, 62 insertions(+), 41 deletions(-) diff --git a/src/storage/src/engine.rs b/src/storage/src/engine.rs index 819662fdc6..fa2552f954 100644 --- a/src/storage/src/engine.rs +++ b/src/storage/src/engine.rs @@ -273,7 +273,6 @@ impl EngineInner { } // Now the region in under `Creating` state. - let region_id = descriptor.id; let region_name = descriptor.name.clone(); let mut guard = SlotGuard::new(®ion_name, &self.regions); @@ -285,13 +284,7 @@ impl EngineInner { })?; let store_config = self.region_store_config(®ion_name); - let region = RegionImpl::create( - region_id, - region_name.clone(), - metadata.clone(), - store_config, - ) - .await?; + let region = RegionImpl::create(metadata, store_config).await?; guard.update(RegionSlot::Ready(region.clone())); diff --git a/src/storage/src/flush.rs b/src/storage/src/flush.rs index 9a5d18ae8a..cc21e85f2e 100644 --- a/src/storage/src/flush.rs +++ b/src/storage/src/flush.rs @@ -72,7 +72,7 @@ impl FlushStrategy for SizeBasedStrategy { logging::info!( "Region should flush, region: {}, bytes_mutable: {}, mutable_limitation: {}, \ bytes_total: {}, max_write_buffer_size: {} .", - shared.name, + shared.name(), bytes_mutable, self.mutable_limitation, bytes_total, @@ -93,7 +93,7 @@ impl FlushStrategy for SizeBasedStrategy { logging::info!( "Region should flush, region: {}, bytes_mutable: {}, mutable_limitation: {}, \ bytes_total: {}, max_write_buffer_size: {} .", - shared.name, + shared.name(), bytes_mutable, self.mutable_limitation, bytes_total, diff --git a/src/storage/src/metadata.rs b/src/storage/src/metadata.rs index 900df11905..b9df711905 100644 --- a/src/storage/src/metadata.rs +++ b/src/storage/src/metadata.rs @@ -59,7 +59,11 @@ pub type VersionNumber = u32; /// In memory metadata of region. #[derive(Clone, Debug, Serialize, Deserialize, PartialEq)] pub struct RegionMetadata { - pub id: RegionId, + // The following fields are immutable. + id: RegionId, + name: String, + + // The following fields are mutable. /// Schema of the region. /// /// Holding a [SchemaRef] to allow converting into `SchemaRef`/`arrow::SchemaRef` @@ -74,6 +78,18 @@ pub struct RegionMetadata { pub version: VersionNumber, } +impl RegionMetadata { + #[inline] + pub fn id(&self) -> RegionId { + self.id + } + + #[inline] + pub fn name(&self) -> &str { + &self.name + } +} + pub type RegionMetadataRef = Arc; #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] @@ -161,6 +177,7 @@ impl TryFrom for RegionMetadata { // Doesn't set version explicitly here, because this is a new region meta // created from descriptor, using initial version is reasonable. let mut builder = RegionMetadataBuilder::new() + .name(desc.name) .id(desc.id) .row_key(desc.row_key)? .add_column_family(desc.default_cf)?; @@ -175,6 +192,7 @@ impl TryFrom for RegionMetadata { #[derive(Default)] struct RegionMetadataBuilder { id: RegionId, + name: String, columns: Vec, column_schemas: Vec, name_to_col_index: HashMap, @@ -190,6 +208,11 @@ impl RegionMetadataBuilder { RegionMetadataBuilder::default() } + fn name(mut self, name: impl Into) -> Self { + self.name = name.into(); + self + } + fn id(mut self, id: RegionId) -> Self { self.id = id; self @@ -271,6 +294,7 @@ impl RegionMetadataBuilder { Ok(RegionMetadata { id: self.id, + name: self.name, schema, columns_row_key, column_families: ColumnFamiliesMetadata { @@ -331,9 +355,12 @@ mod tests { use crate::test_util::descriptor_util::RegionDescBuilder; use crate::test_util::schema_util; + const TEST_REGION: &str = "test-region"; + #[test] fn test_descriptor_to_region_metadata() { - let desc = RegionDescBuilder::new("region-0") + let region_name = "region-0"; + let desc = RegionDescBuilder::new(region_name) .timestamp(("ts", LogicalTypeId::Int64, false)) .enable_version_column(false) .push_key_column(("k1", LogicalTypeId::Int32, false)) @@ -350,6 +377,7 @@ mod tests { ); let metadata = RegionMetadata::try_from(desc).unwrap(); + assert_eq!(region_name, metadata.name); assert_eq!(expect_schema, metadata.schema); assert_eq!(2, metadata.columns_row_key.num_row_key_columns()); assert_eq!(1, metadata.columns_row_key.num_value_columns()); @@ -403,6 +431,7 @@ mod tests { .build() .unwrap(); RegionMetadataBuilder::new() + .name(TEST_REGION) .row_key(row_key) .unwrap() .add_column_family(cf) @@ -414,6 +443,7 @@ mod tests { #[test] fn test_build_metedata_disable_version() { let metadata = new_metadata(false); + assert_eq!(TEST_REGION, metadata.name); let expect_schema = schema_util::new_schema_ref( &[ @@ -460,6 +490,7 @@ mod tests { #[test] fn test_build_metedata_enable_version() { let metadata = new_metadata(true); + assert_eq!(TEST_REGION, metadata.name); let expect_schema = schema_util::new_schema_ref( &[ diff --git a/src/storage/src/region.rs b/src/storage/src/region.rs index 5831b039f5..eaee5d6970 100644 --- a/src/storage/src/region.rs +++ b/src/storage/src/region.rs @@ -91,11 +91,7 @@ impl RegionImpl { /// Create a new region and also persist the region metadata to manifest. /// /// The caller should avoid calling this method simultaneously. - // FIXME(yingwen): Region id is already specific in metadata, but name is not specific in metadata. We should - // add name to RegionMetadata. pub async fn create( - id: RegionId, - name: String, metadata: RegionMetadata, store_config: StoreConfig, ) -> Result> { @@ -113,18 +109,16 @@ impl RegionImpl { .await?; let version = Version::with_manifest_version(metadata, manifest_version); - let region = RegionImpl::new(id, name, version, store_config); + let region = RegionImpl::new(version, store_config); Ok(region) } /// Create a new region without persisting manifest. - fn new( - id: RegionId, - name: String, - version: Version, - store_config: StoreConfig, - ) -> RegionImpl { + fn new(version: Version, store_config: StoreConfig) -> RegionImpl { + let metadata = version.metadata(); + let id = metadata.id(); + let name = metadata.name().to_string(); let version_control = VersionControl::with_version(version); let wal = Wal::new(name.clone(), store_config.log_store); @@ -165,7 +159,7 @@ impl RegionImpl { let version_control = Arc::new(VersionControl::with_version(version)); let wal = Wal::new(name.clone(), store_config.log_store); let shared = Arc::new(SharedData { - id: metadata.id, + id: metadata.id(), name, version_control, }); @@ -288,12 +282,26 @@ impl RegionImpl { /// Shared data of region. #[derive(Debug)] pub struct SharedData { - pub id: RegionId, - pub name: String, + // Region id and name is immutable, so we cache them in shared data to avoid loading + // current version from `version_control` each time we need to access them. + id: RegionId, + name: String, // TODO(yingwen): Maybe no need to use Arc for version control. pub version_control: VersionControlRef, } +impl SharedData { + #[inline] + pub fn id(&self) -> RegionId { + self.id + } + + #[inline] + pub fn name(&self) -> &str { + &self.name + } +} + pub type SharedDataRef = Arc; #[derive(Debug)] diff --git a/src/storage/src/region/tests.rs b/src/storage/src/region/tests.rs index 30e8a8aa87..ce1fbf0946 100644 --- a/src/storage/src/region/tests.rs +++ b/src/storage/src/region/tests.rs @@ -155,12 +155,7 @@ async fn test_new_region() { let store_config = config_util::new_store_config(region_name, &store_dir).await; - let region = RegionImpl::new( - 0, - region_name.to_string(), - Version::new(Arc::new(metadata)), - store_config, - ); + let region = RegionImpl::new(Version::new(Arc::new(metadata)), store_config); let expect_schema = schema_util::new_schema_ref( &[ diff --git a/src/storage/src/region/tests/basic.rs b/src/storage/src/region/tests/basic.rs index 7d71968be5..29a7d37772 100644 --- a/src/storage/src/region/tests/basic.rs +++ b/src/storage/src/region/tests/basic.rs @@ -21,9 +21,7 @@ async fn create_region_for_basic( let store_config = config_util::new_store_config(region_name, store_dir).await; - RegionImpl::create(0, region_name.to_string(), metadata, store_config) - .await - .unwrap() + RegionImpl::create(metadata, store_config).await.unwrap() } /// Tester for basic tests. diff --git a/src/storage/src/region/tests/flush.rs b/src/storage/src/region/tests/flush.rs index 7bc20d0a10..ef784779a9 100644 --- a/src/storage/src/region/tests/flush.rs +++ b/src/storage/src/region/tests/flush.rs @@ -26,9 +26,7 @@ async fn create_region_for_flush( let mut store_config = config_util::new_store_config(REGION_NAME, store_dir).await; store_config.flush_strategy = flush_strategy; - RegionImpl::create(0, REGION_NAME.to_string(), metadata, store_config) - .await - .unwrap() + RegionImpl::create(metadata, store_config).await.unwrap() } /// Tester for region flush. diff --git a/src/store-api/src/storage/engine.rs b/src/store-api/src/storage/engine.rs index 5b2096453d..db97d5d2e1 100644 --- a/src/store-api/src/storage/engine.rs +++ b/src/store-api/src/storage/engine.rs @@ -64,6 +64,4 @@ pub struct EngineContext {} /// Options to open a region. #[derive(Debug, Clone, Default)] -pub struct OpenOptions { - // TODO(yingwen): [open_region] Supports create if not exists. -} +pub struct OpenOptions {}