Skip to content

Commit

Permalink
refactor: Define DatabaseIdToNameIdent with TIdent
Browse files Browse the repository at this point in the history
  • Loading branch information
drmingdrmer committed May 12, 2024
1 parent c221094 commit 28b0574
Show file tree
Hide file tree
Showing 15 changed files with 160 additions and 92 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion src/meta/api/src/schema_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use databend_common_meta_app::schema::CreateTableReply;
use databend_common_meta_app::schema::CreateTableReq;
use databend_common_meta_app::schema::CreateVirtualColumnReply;
use databend_common_meta_app::schema::CreateVirtualColumnReq;
use databend_common_meta_app::schema::DatabaseIdIdent;
use databend_common_meta_app::schema::DatabaseInfo;
use databend_common_meta_app::schema::DeleteLockRevReq;
use databend_common_meta_app::schema::DropCatalogReply;
Expand Down Expand Up @@ -223,7 +224,7 @@ pub trait SchemaApi: Send + Sync {
db_ids: &[MetaId],
) -> Result<Vec<Option<String>>, KVAppError>;

async fn get_db_name_by_id(&self, db_id: MetaId) -> Result<String, KVAppError>;
async fn get_db_name_by_id(&self, db_id: DatabaseIdIdent) -> Result<String, KVAppError>;

async fn get_table_copied_file_info(
&self,
Expand Down
23 changes: 12 additions & 11 deletions src/meta/api/src/schema_api_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ use databend_common_meta_app::schema::CreateVirtualColumnReq;
use databend_common_meta_app::schema::DBIdTableName;
use databend_common_meta_app::schema::DatabaseIdHistoryIdent;
use databend_common_meta_app::schema::DatabaseIdIdent;
use databend_common_meta_app::schema::DatabaseIdToName;
use databend_common_meta_app::schema::DatabaseIdToNameIdent;
use databend_common_meta_app::schema::DatabaseIdent;
use databend_common_meta_app::schema::DatabaseInfo;
use databend_common_meta_app::schema::DatabaseInfoFilter;
Expand Down Expand Up @@ -322,7 +322,7 @@ impl<KV: kvapi::KVApi<Error = MetaError> + ?Sized> SchemaApi for KV {

let db_id = fetch_id(self, IdGenerator::database_id()).await?;
let db_id_ident = DatabaseIdIdent::new(&tenant, db_id);
let id_to_name_key = DatabaseIdToName { db_id };
let id_to_name_key = DatabaseIdToNameIdent::new(&tenant, db_id);

debug!(db_id = db_id, name_key :? =(name_key); "new database id");

Expand Down Expand Up @@ -520,6 +520,7 @@ impl<KV: kvapi::KVApi<Error = MetaError> + ?Sized> SchemaApi for KV {
) -> Result<RenameDatabaseReply, KVAppError> {
debug!(req :? =(&req); "SchemaApi: {}", func_name!());

let tenant = req.name_ident.tenant().clone();
let tenant_dbname = &req.name_ident;
let tenant_newdbname = DatabaseNameIdent::new(tenant_dbname.tenant(), &req.new_db_name);

Expand Down Expand Up @@ -548,7 +549,7 @@ impl<KV: kvapi::KVApi<Error = MetaError> + ?Sized> SchemaApi for KV {
db_has_to_not_exist(db_id_seq, &tenant_newdbname, "rename_database")?;

// get db id -> name
let db_id_key = DatabaseIdToName { db_id: old_db_id };
let db_id_key = DatabaseIdToNameIdent::new(&tenant, old_db_id);
let (db_name_seq, _): (_, Option<DatabaseNameIdentRaw>) =
get_pb_value(self, &db_id_key).await?;

Expand Down Expand Up @@ -2262,19 +2263,19 @@ impl<KV: kvapi::KVApi<Error = MetaError> + ?Sized> SchemaApi for KV {

#[logcall::logcall("debug")]
#[minitrace::trace]
async fn get_db_name_by_id(&self, db_id: u64) -> Result<String, KVAppError> {
debug!(req :? =(&db_id); "SchemaApi: {}", func_name!());
async fn get_db_name_by_id(&self, db_id_ident: DatabaseIdIdent) -> Result<String, KVAppError> {
debug!(req :? =(&db_id_ident); "SchemaApi: {}", func_name!());

let db_id_to_name_key = DatabaseIdToName { db_id };
let db_id_to_name_ident = DatabaseIdToNameIdent::new_from(db_id_ident.clone());

let (meta_seq, db_name): (_, Option<DatabaseNameIdentRaw>) =
get_pb_value(self, &db_id_to_name_key).await?;
get_pb_value(self, &db_id_to_name_ident).await?;

debug!(ident :% =(&db_id_to_name_key); "get_db_name_by_id");
debug!(ident :% =(&db_id_to_name_ident.display()); "get_db_name_by_id");

if meta_seq == 0 || db_name.is_none() {
return Err(KVAppError::AppError(AppError::UnknownDatabaseId(
UnknownDatabaseId::new(db_id, "get_db_name_by_id"),
UnknownDatabaseId::new(db_id_ident.database_id(), "get_db_name_by_id"),
)));
}

Expand All @@ -2292,7 +2293,7 @@ impl<KV: kvapi::KVApi<Error = MetaError> + ?Sized> SchemaApi for KV {

let mut kv_keys = Vec::with_capacity(db_ids.len());
for id in db_ids {
let k = DatabaseIdToName { db_id: *id }.to_string_key();
let k = DatabaseIdToNameIdent::new(tenant, *id).to_string_key();
kv_keys.push(k);
}

Expand Down Expand Up @@ -4684,7 +4685,7 @@ async fn gc_dropped_db_by_id(
if db_meta_seq == 0 {
return Ok(());
}
let id_to_name = DatabaseIdToName { db_id };
let id_to_name = DatabaseIdToNameIdent::new(tenant, db_id);
let (name_ident_seq, _name_ident): (_, Option<DatabaseNameIdentRaw>) =
get_pb_value(kv_api, &id_to_name).await?;
if name_ident_seq == 0 {
Expand Down
28 changes: 17 additions & 11 deletions src/meta/api/src/schema_api_test_suite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ use databend_common_meta_app::schema::CreateVirtualColumnReq;
use databend_common_meta_app::schema::DBIdTableName;
use databend_common_meta_app::schema::DatabaseIdHistoryIdent;
use databend_common_meta_app::schema::DatabaseIdIdent;
use databend_common_meta_app::schema::DatabaseIdToName;
use databend_common_meta_app::schema::DatabaseIdToNameIdent;
use databend_common_meta_app::schema::DatabaseInfo;
use databend_common_meta_app::schema::DatabaseMeta;
use databend_common_meta_app::schema::DbIdList;
Expand Down Expand Up @@ -470,7 +470,7 @@ impl SchemaApiTestSuite {
let res = mt.create_table(req).await?;
table_id = res.table_id;

let db_id_name_key = DatabaseIdToName { db_id };
let db_id_name_key = DatabaseIdToNameIdent::new(&tenant, db_id);
let ret_db_name_ident: DatabaseNameIdentRaw =
get_kv_data(mt.as_kv_api(), &db_id_name_key).await?;
assert_eq!(
Expand Down Expand Up @@ -498,7 +498,7 @@ impl SchemaApiTestSuite {
info!("rename database res: {:?}", res);
assert!(res.is_ok());

let db_id_2_name_key = DatabaseIdToName { db_id };
let db_id_2_name_key = DatabaseIdToNameIdent::new(&tenant, db_id);
let ret_db_name_ident: DatabaseNameIdentRaw =
get_kv_data(mt.as_kv_api(), &db_id_2_name_key).await?;
assert_eq!(
Expand Down Expand Up @@ -724,7 +724,7 @@ impl SchemaApiTestSuite {

let orig_db_id: u64 = get_kv_u64_data(mt.as_kv_api(), &db_name).await?;
assert_eq!(orig_db_id, res.ident.db_id);
let db_id_name_key = DatabaseIdToName { db_id: orig_db_id };
let db_id_name_key = DatabaseIdToNameIdent::new(&tenant, orig_db_id);
let ret_db_name_ident: DatabaseNameIdentRaw =
get_kv_data(mt.as_kv_api(), &db_id_name_key).await?;
assert_eq!(ret_db_name_ident, DatabaseNameIdentRaw::from(&db_name));
Expand All @@ -748,7 +748,7 @@ impl SchemaApiTestSuite {

let db_id: u64 = get_kv_u64_data(mt.as_kv_api(), &db_name).await?;
assert_eq!(db_id, res.ident.db_id);
let db_id_name_key = DatabaseIdToName { db_id };
let db_id_name_key = DatabaseIdToNameIdent::new(&tenant, db_id);
let ret_db_name_ident: DatabaseNameIdentRaw =
get_kv_data(mt.as_kv_api(), &db_id_name_key).await?;
assert_eq!(ret_db_name_ident, DatabaseNameIdentRaw::from(&db_name));
Expand Down Expand Up @@ -2038,7 +2038,7 @@ impl SchemaApiTestSuite {

info!("--- drop db-id-to-name mapping to ensure dropping table does not rely on it");
{
let id_to_name_key = DatabaseIdToName { db_id: util.db_id };
let id_to_name_key = DatabaseIdToNameIdent::new(&util.tenant, util.db_id);
util.mt
.as_kv_api()
.upsert_kv(UpsertKV::delete(id_to_name_key.to_string_key()))
Expand Down Expand Up @@ -3384,7 +3384,7 @@ impl SchemaApiTestSuite {
// assert old db meta and id to name mapping has been removed
for db_id in old_id_list.iter() {
let id_key = DatabaseIdIdent::new(&tenant, *db_id);
let id_mapping = DatabaseIdToName { db_id: *db_id };
let id_mapping = DatabaseIdToNameIdent::new(&tenant, *db_id);

let meta_res: Result<DatabaseMeta, KVAppError> =
get_kv_data(mt.as_kv_api(), &id_key).await;
Expand Down Expand Up @@ -3767,7 +3767,7 @@ impl SchemaApiTestSuite {
// assert old db meta and id to name mapping has been removed
for db_id in old_id_list.id_list.iter() {
let id_key = DatabaseIdIdent::new(&tenant, *db_id);
let id_mapping = DatabaseIdToName { db_id: *db_id };
let id_mapping = DatabaseIdToNameIdent::new(&tenant, *db_id);

let meta_res: Result<DatabaseMeta, KVAppError> =
get_kv_data(mt.as_kv_api(), &id_key).await;
Expand Down Expand Up @@ -4981,7 +4981,9 @@ impl SchemaApiTestSuite {

assert_eq!(1, res.db_id, "first database id is 1");

let got = mt.get_db_name_by_id(res.db_id).await?;
let got = mt
.get_db_name_by_id(DatabaseIdIdent::new(&tenant, res.db_id))
.await?;
assert_eq!(got, db_name.to_string())
}

Expand All @@ -4995,14 +4997,18 @@ impl SchemaApiTestSuite {

let db = mt.get_database(plan).await.unwrap();

let got = mt.get_db_name_by_id(db.ident.db_id).await?;
let got = mt
.get_db_name_by_id(DatabaseIdIdent::new(&tenant, db.ident.db_id))
.await?;

assert_eq!(got, db_name.to_string());
}

info!("--- get_db_name_by_id with not exists db_id");
{
let got = mt.get_db_name_by_id(1024).await;
let got = mt
.get_db_name_by_id(DatabaseIdIdent::new(&tenant, 1024))
.await;

let err = got.unwrap_err();
let err = ErrorCode::from(err);
Expand Down
55 changes: 0 additions & 55 deletions src/meta/app/src/schema/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,23 +42,6 @@ pub struct DatabaseIdent {
pub seq: u64,
}

#[derive(Clone, Debug, Default, Eq, PartialEq)]
pub struct DatabaseIdToName {
pub db_id: u64,
}

impl Display for DatabaseIdToName {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.db_id)
}
}

impl DatabaseIdToName {
pub fn new(db_id: u64) -> Self {
DatabaseIdToName { db_id }
}
}

#[derive(Clone, Debug, Eq, PartialEq)]
pub struct DatabaseMeta {
pub engine: String,
Expand Down Expand Up @@ -299,41 +282,3 @@ impl ListDatabaseReq {
&self.tenant
}
}

mod kvapi_key_impl {
use databend_common_meta_kvapi::kvapi;

use crate::schema::database_name_ident::DatabaseNameIdentRaw;
use crate::schema::DatabaseIdIdent;
use crate::schema::DatabaseIdToName;
use crate::tenant::Tenant;

impl kvapi::KeyCodec for DatabaseIdToName {
fn encode_key(&self, b: kvapi::KeyBuilder) -> kvapi::KeyBuilder {
b.push_u64(self.db_id)
}

fn decode_key(parser: &mut kvapi::KeyParser) -> Result<Self, kvapi::KeyError> {
let db_id = parser.next_u64()?;
Ok(Self { db_id })
}
}

/// "__fd_database_id_to_name/<db_id> -> DatabaseNameIdent"
impl kvapi::Key for DatabaseIdToName {
const PREFIX: &'static str = "__fd_database_id_to_name";

type ValueType = DatabaseNameIdentRaw;

fn parent(&self) -> Option<String> {
// TODO(TIdent): add real tenant
Some(DatabaseIdIdent::new(Tenant::new_literal("dummy"), self.db_id).to_string_key())
}
}

impl kvapi::Value for DatabaseNameIdentRaw {
fn dependency_keys(&self) -> impl IntoIterator<Item = String> {
[]
}
}
}
91 changes: 91 additions & 0 deletions src/meta/app/src/schema/database_id_to_name_ident.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// Copyright 2021 Datafuse Labs
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::tenant_key::ident::TIdent;
use crate::tenant_key::raw::TIdentRaw;

pub type DatabaseIdToNameIdent = TIdent<Resource, u64>;
pub type DatabaseIdToNameIdentRaw = TIdentRaw<Resource, u64>;

pub use kvapi_impl::Resource;

impl DatabaseIdToNameIdent {
pub fn database_id(&self) -> u64 {
*self.name()
}
}

impl DatabaseIdToNameIdentRaw {
pub fn database_id(&self) -> u64 {
*self.name()
}
}

mod kvapi_impl {

use databend_common_meta_kvapi::kvapi;

use crate::schema::database_name_ident::DatabaseNameIdentRaw;
use crate::tenant_key::resource::TenantResource;

pub struct Resource;
impl TenantResource for Resource {
const PREFIX: &'static str = "__fd_database_id_to_name";
const TYPE: &'static str = "DatabaseIdToNameIdent";
const HAS_TENANT: bool = false;
type ValueType = DatabaseNameIdentRaw;
}

// TODO(TIdent): parent: DatabaseIdIdent
impl kvapi::Value for DatabaseNameIdentRaw {
fn dependency_keys(&self) -> impl IntoIterator<Item = String> {
[]
}
}

// // Use these error types to replace usage of ErrorCode if possible.
// impl From<ExistError<Resource>> for ErrorCode {
// impl From<UnknownError<Resource>> for ErrorCode {
}

#[cfg(test)]
mod tests {
use databend_common_meta_kvapi::kvapi::Key;

use super::DatabaseIdToNameIdent;
use crate::tenant::Tenant;

#[test]
fn test_background_job_id_ident() {
let tenant = Tenant::new_literal("dummy");
let ident = DatabaseIdToNameIdent::new(tenant, 3);

let key = ident.to_string_key();
assert_eq!(key, "__fd_database_id_to_name/3");

assert_eq!(ident, DatabaseIdToNameIdent::from_str_key(&key).unwrap());
}

#[test]
fn test_background_job_id_ident_with_key_space() {
// TODO(xp): implement this test
// let tenant = Tenant::new_literal("test");
// let ident = DatabaseIdToNameIdent::new(tenant, 3);
//
// let key = ident.to_string_key();
// assert_eq!(key, "__fd_database_id_to_name/3");
//
// assert_eq!(ident, DatabaseIdToNameIdent::from_str_key(&key).unwrap());
}
}
3 changes: 2 additions & 1 deletion src/meta/app/src/schema/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ pub mod catalog_id_to_name_ident;
pub mod catalog_name_ident;
pub mod database_id_history_ident;
pub mod database_id_ident;
pub mod database_id_to_name_ident;
pub mod database_name_ident;
pub mod index_name_ident;
pub mod table_lock_ident;
Expand All @@ -42,7 +43,6 @@ pub use catalog_name_ident::CatalogNameIdent;
pub use create_option::CreateOption;
pub use database::CreateDatabaseReply;
pub use database::CreateDatabaseReq;
pub use database::DatabaseIdToName;
pub use database::DatabaseIdent;
pub use database::DatabaseInfo;
pub use database::DatabaseInfoFilter;
Expand All @@ -58,6 +58,7 @@ pub use database::UndropDatabaseReply;
pub use database::UndropDatabaseReq;
pub use database_id_history_ident::DatabaseIdHistoryIdent;
pub use database_id_ident::DatabaseIdIdent;
pub use database_id_to_name_ident::DatabaseIdToNameIdent;
pub use index::*;
pub use index_name_ident::IndexNameIdent;
pub use index_name_ident::IndexNameIdentRaw;
Expand Down
Loading

0 comments on commit 28b0574

Please sign in to comment.