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 {}