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

@@ -273,7 +273,6 @@ impl<S: LogStore> EngineInner<S> {
         }
 
         // Now the region in under `Creating` state.
-        let region_id = descriptor.id;
         let region_name = descriptor.name.clone();
         let mut guard = SlotGuard::new(&region_name, &self.regions);
 
@@ -285,13 +284,7 @@ impl<S: LogStore> EngineInner<S> {
                 })?;
         let store_config = self.region_store_config(&region_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()));
 

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,

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<RegionMetadata>;
 
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
@@ -161,6 +177,7 @@ impl TryFrom<RegionDescriptor> 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<RegionDescriptor> for RegionMetadata {
 #[derive(Default)]
 struct RegionMetadataBuilder {
     id: RegionId,
+    name: String,
     columns: Vec<ColumnMetadata>,
     column_schemas: Vec<ColumnSchema>,
     name_to_col_index: HashMap<String, usize>,
@@ -190,6 +208,11 @@ impl RegionMetadataBuilder {
         RegionMetadataBuilder::default()
     }
 
+    fn name(mut self, name: impl Into<String>) -> 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(
             &[

src/storage/src/region.rs

@@ -91,11 +91,7 @@ impl<S: LogStore> RegionImpl<S> {
     /// 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<S>,
     ) -> Result<RegionImpl<S>> {
@@ -113,18 +109,16 @@ impl<S: LogStore> RegionImpl<S> {
             .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<S>,
-    ) -> RegionImpl<S> {
+    fn new(version: Version, store_config: StoreConfig<S>) -> RegionImpl<S> {
+        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<S: LogStore> RegionImpl<S> {
         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<S: LogStore> RegionImpl<S> {
 /// 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<SharedData>;
 
 #[derive(Debug)]

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(
         &[

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.

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.

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