From 2ab16680e9ac482cdb43dd56b80c52fbd5d6abcd Mon Sep 17 00:00:00 2001 From: Georgy Moshkin <gmoshkin@picodata.io> Date: Wed, 13 Sep 2023 14:15:15 +0300 Subject: [PATCH] refactor: remove ClusterwideSpaceId --- src/cas.rs | 24 +++++----- src/governor/plan.rs | 14 +++--- src/lib.rs | 14 +++--- src/rpc/join.rs | 14 +++--- src/rpc/update_instance.rs | 10 ++-- src/schema.rs | 12 ++--- src/storage.rs | 95 +++++++++++++------------------------- src/traft/node.rs | 9 ++-- 8 files changed, 79 insertions(+), 113 deletions(-) diff --git a/src/cas.rs b/src/cas.rs index 62e4c0abda..9875cd4a53 100644 --- a/src/cas.rs +++ b/src/cas.rs @@ -2,7 +2,7 @@ use std::time::Duration; use crate::rpc; use crate::storage::Clusterwide; -use crate::storage::ClusterwideSpaceId; +use crate::storage::ClusterwideSpace; use crate::tlog; use crate::traft; use crate::traft::error::Error as TraftError; @@ -28,12 +28,12 @@ use tarantool::tuple::{KeyDef, ToTupleBuffer, Tuple, TupleBuffer}; /// This spaces cannot be changed directly dy a [`Dml`] operation. They have /// dedicated operation types (e.g. Ddl, Acl) because updating these spaces /// requires automatically updating corresponding local spaces. -const PROHIBITED_SPACES: &[ClusterwideSpaceId] = &[ - ClusterwideSpaceId::Space, - ClusterwideSpaceId::Index, - ClusterwideSpaceId::User, - ClusterwideSpaceId::Role, - ClusterwideSpaceId::Privilege, +const PROHIBITED_SPACES: &[ClusterwideSpace] = &[ + ClusterwideSpace::Space, + ClusterwideSpace::Index, + ClusterwideSpace::User, + ClusterwideSpace::Role, + ClusterwideSpace::Privilege, ]; // FIXME: cas::Error will be returned as a string when rpc is called @@ -185,7 +185,7 @@ fn proc_cas_local(req: Request) -> Result<Response> { // Check if ranges in predicate contain prohibited spaces. for range in &req.predicate.ranges { - let Ok(space) = ClusterwideSpaceId::try_from(range.space) else { continue; }; + let Ok(space) = ClusterwideSpace::try_from(range.space) else { continue; }; if PROHIBITED_SPACES.contains(&space) { return Err(Error::SpaceNotAllowed { space: space.name().into(), @@ -482,7 +482,7 @@ pub fn schema_change_ranges() -> &'static [Range] { if DATA.is_none() { let mut data = Vec::with_capacity(SCHEMA_RELATED_PROPERTIES.len()); for key in SCHEMA_RELATED_PROPERTIES { - let r = Range::new(ClusterwideSpaceId::Property).eq((key,)); + let r = Range::new(ClusterwideSpace::Property).eq((key,)); data.push(r); } DATA = Some(data); @@ -662,7 +662,7 @@ fn space(op: &Op) -> Option<SpaceId> { match op { Op::Dml(dml) => Some(dml.space()), Op::DdlPrepare { .. } | Op::DdlCommit | Op::DdlAbort | Op::Acl { .. } => { - Some(ClusterwideSpaceId::Property.into()) + Some(ClusterwideSpace::Property.id()) } Op::Nop => None, } @@ -836,7 +836,7 @@ mod tests { predicate.check_entry(2, &Op::Dml(op.clone()), &storage) }; - let space = ClusterwideSpaceId::Space; + let space = ClusterwideSpace::Space; let ops = &[ Dml::Insert { space: space.into(), @@ -857,7 +857,7 @@ mod tests { }, ]; - let space = ClusterwideSpaceId::Space.value(); + let space = ClusterwideSpace::Space.id(); for op in ops { assert!(test(op, Range::new(space)).is_err()); assert!(test(op, Range::new(space).le((12,))).is_err()); diff --git a/src/governor/plan.rs b/src/governor/plan.rs index 458acae1cd..58fa6c3e08 100644 --- a/src/governor/plan.rs +++ b/src/governor/plan.rs @@ -4,7 +4,7 @@ use crate::instance::{Instance, InstanceId}; use crate::replicaset::weight; use crate::replicaset::{Replicaset, ReplicasetId}; use crate::rpc; -use crate::storage::{ClusterwideSpaceId, PropertyName}; +use crate::storage::{ClusterwideSpace, PropertyName}; use crate::tlog; use crate::traft::op::Dml; use crate::traft::Result; @@ -82,7 +82,7 @@ pub(super) fn action_plan<'i>( let rpc = rpc::replication::promote::Request {}; let mut ops = UpdateOps::new(); ops.assign("master_id", &to.instance_id)?; - let op = Dml::update(ClusterwideSpaceId::Replicaset, &[&to.replicaset_id], ops)?; + let op = Dml::update(ClusterwideSpace::Replicaset, &[&to.replicaset_id], ops)?; return Ok(TransferMastership { to, rpc, op }.into()); } else { tlog!(Warning, "replicaset master is going offline and no substitution is found"; @@ -127,7 +127,7 @@ pub(super) fn action_plan<'i>( { let rpc = rpc::replication::promote::Request {}; let op = Dml::insert( - ClusterwideSpaceId::Replicaset, + ClusterwideSpace::Replicaset, &Replicaset { replicaset_id: replicaset_id.clone(), replicaset_uuid: replicaset_uuid.clone(), @@ -150,7 +150,7 @@ pub(super) fn action_plan<'i>( let rpc = rpc::replication::promote::Request {}; let mut ops = UpdateOps::new(); ops.assign("master_id", &to.instance_id)?; - let op = Dml::update(ClusterwideSpaceId::Replicaset, &[&to.replicaset_id], ops)?; + let op = Dml::update(ClusterwideSpace::Replicaset, &[&to.replicaset_id], ops)?; return Ok(TransferMastership { to, rpc, op }.into()); } @@ -245,7 +245,7 @@ pub(super) fn action_plan<'i>( timeout: Loop::SYNC_TIMEOUT, }; let op = Dml::replace( - ClusterwideSpaceId::Property, + ClusterwideSpace::Property, &(PropertyName::VshardBootstrapped, true), )?; return Ok(ShardingBoot { target, rpc, op }.into()); @@ -266,7 +266,7 @@ pub(super) fn action_plan<'i>( weight::State::UpToDate }; uops.assign(weight::State::PATH, state)?; - let op = Dml::update(ClusterwideSpaceId::Replicaset, &[replicaset_id], uops)?; + let op = Dml::update(ClusterwideSpace::Replicaset, &[replicaset_id], uops)?; return Ok(ProposeWeightChanges { op }.into()); } @@ -309,7 +309,7 @@ pub(super) fn action_plan<'i>( for replicaset_id in to_update_weights { let mut uops = UpdateOps::new(); uops.assign(weight::State::PATH, weight::State::UpToDate)?; - let op = Dml::update(ClusterwideSpaceId::Replicaset, &[replicaset_id], uops)?; + let op = Dml::update(ClusterwideSpace::Replicaset, &[replicaset_id], uops)?; ops.push(op); } return Ok(UpdateWeights { targets, rpc, ops }.into()); diff --git a/src/lib.rs b/src/lib.rs index b0ffd4c700..d700bc5ea2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,7 +14,7 @@ use std::convert::TryFrom; use std::time::{Duration, Instant}; use storage::tweak_max_space_id; use storage::Clusterwide; -use storage::{ClusterwideSpaceId, PropertyName}; +use storage::{ClusterwideSpace, PropertyName}; use traft::RaftSpaceAccess; use protobuf::Message as _; @@ -428,7 +428,7 @@ fn start_boot(args: &args::Run) { init_entries_push_op( op::Dml::insert( - ClusterwideSpaceId::Address, + ClusterwideSpace::Address, &traft::PeerAddress { raft_id, address: args.advertise_address(), @@ -438,13 +438,13 @@ fn start_boot(args: &args::Run) { .into(), ); init_entries_push_op( - op::Dml::insert(ClusterwideSpaceId::Instance, &instance) + op::Dml::insert(ClusterwideSpace::Instance, &instance) .expect("cannot fail") .into(), ); init_entries_push_op( op::Dml::insert( - ClusterwideSpaceId::Property, + ClusterwideSpace::Property, &( PropertyName::ReplicationFactor, args.init_replication_factor, @@ -455,7 +455,7 @@ fn start_boot(args: &args::Run) { ); init_entries_push_op( op::Dml::insert( - ClusterwideSpaceId::Property, + ClusterwideSpace::Property, &(PropertyName::GlobalSchemaVersion, 0), ) .expect("cannot fail") @@ -463,7 +463,7 @@ fn start_boot(args: &args::Run) { ); init_entries_push_op( op::Dml::insert( - ClusterwideSpaceId::Property, + ClusterwideSpace::Property, &(PropertyName::NextSchemaVersion, 1), ) .expect("cannot fail") @@ -472,7 +472,7 @@ fn start_boot(args: &args::Run) { init_entries_push_op( op::Dml::insert( - ClusterwideSpaceId::Property, + ClusterwideSpace::Property, &(PropertyName::PasswordMinLength, INITIAL_PASSWORD_MIN_LENGTH), ) .expect("cannot fail") diff --git a/src/rpc/join.rs b/src/rpc/join.rs index f9a7cb508e..c9957ddd8e 100644 --- a/src/rpc/join.rs +++ b/src/rpc/join.rs @@ -8,7 +8,7 @@ use crate::instance::grade::{CurrentGrade, TargetGrade}; use crate::instance::{Instance, InstanceId}; use crate::replicaset::ReplicasetId; use crate::storage::{Clusterwide, ToEntryIter as _}; -use crate::storage::{ClusterwideSpaceId, PropertyName}; +use crate::storage::{ClusterwideSpace, PropertyName}; use crate::traft::op::{Dml, Op}; use crate::traft::{self, RaftId}; use crate::traft::{error::Error, node, Address, PeerAddress, Result}; @@ -88,14 +88,14 @@ pub fn handle_join_request_and_wait(req: Request, timeout: Duration) -> Result<R raft_id: instance.raft_id, address: req.advertise_address.clone(), }; - let op_addr = Dml::replace(ClusterwideSpaceId::Address, &peer_address) - .expect("encoding should not fail"); - let op_instance = Dml::replace(ClusterwideSpaceId::Instance, &instance) + let op_addr = Dml::replace(ClusterwideSpace::Address, &peer_address) .expect("encoding should not fail"); + let op_instance = + Dml::replace(ClusterwideSpace::Instance, &instance).expect("encoding should not fail"); let ranges = vec![ - cas::Range::new(ClusterwideSpaceId::Instance), - cas::Range::new(ClusterwideSpaceId::Address), - cas::Range::new(ClusterwideSpaceId::Property).eq((PropertyName::ReplicationFactor,)), + cas::Range::new(ClusterwideSpace::Instance), + cas::Range::new(ClusterwideSpace::Address), + cas::Range::new(ClusterwideSpace::Property).eq((PropertyName::ReplicationFactor,)), ]; macro_rules! handle_result { ($res:expr) => { diff --git a/src/rpc/update_instance.rs b/src/rpc/update_instance.rs index f7eff464f3..5e03c6ee00 100644 --- a/src/rpc/update_instance.rs +++ b/src/rpc/update_instance.rs @@ -4,7 +4,7 @@ use crate::cas; use crate::failure_domain::FailureDomain; use crate::instance::grade::{CurrentGrade, CurrentGradeVariant, Grade, TargetGradeVariant}; use crate::instance::{Instance, InstanceId}; -use crate::storage::{Clusterwide, ClusterwideSpaceId, PropertyName}; +use crate::storage::{Clusterwide, ClusterwideSpace, PropertyName}; use crate::traft::op::{Dml, Op}; use crate::traft::Result; use crate::traft::{error::Error, node}; @@ -118,13 +118,13 @@ pub fn handle_update_instance_request_and_wait(req: Request, timeout: Duration) return Ok(()); } - let dml = Dml::replace(ClusterwideSpaceId::Instance, &new_instance) + let dml = Dml::replace(ClusterwideSpace::Instance, &new_instance) .expect("encoding should not fail"); let ranges = vec![ - cas::Range::new(ClusterwideSpaceId::Instance), - cas::Range::new(ClusterwideSpaceId::Address), - cas::Range::new(ClusterwideSpaceId::Property).eq((PropertyName::ReplicationFactor,)), + cas::Range::new(ClusterwideSpace::Instance), + cas::Range::new(ClusterwideSpace::Address), + cas::Range::new(ClusterwideSpace::Property).eq((PropertyName::ReplicationFactor,)), ]; let res = cas::compare_and_swap( Op::Dml(dml), diff --git a/src/schema.rs b/src/schema.rs index 33144dd5d7..086da952e9 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -21,7 +21,7 @@ use serde::{Deserialize, Serialize}; use crate::cas::{self, compare_and_swap}; use crate::storage::SPACE_ID_INTERNAL_MAX; -use crate::storage::{Clusterwide, ClusterwideSpaceId, PropertyName}; +use crate::storage::{Clusterwide, ClusterwideSpace, PropertyName}; use crate::traft::error::Error; use crate::traft::op::{Ddl, Op}; use crate::traft::{self, event, node, RaftIndex}; @@ -688,16 +688,14 @@ pub fn abort_ddl(timeout: Duration) -> traft::Result<RaftIndex> { } let index = node.get_index(); let term = raft::Storage::term(&node.raft_storage, index)?; + #[rustfmt::skip] let predicate = cas::Predicate { index, term, ranges: vec![ - cas::Range::new(ClusterwideSpaceId::Property) - .eq((PropertyName::PendingSchemaChange,)), - cas::Range::new(ClusterwideSpaceId::Property) - .eq((PropertyName::GlobalSchemaVersion,)), - cas::Range::new(ClusterwideSpaceId::Property) - .eq((PropertyName::NextSchemaVersion,)), + cas::Range::new(ClusterwideSpace::Property).eq([PropertyName::PendingSchemaChange]), + cas::Range::new(ClusterwideSpace::Property).eq([PropertyName::GlobalSchemaVersion]), + cas::Range::new(ClusterwideSpace::Property).eq([PropertyName::NextSchemaVersion]), ], }; let (index, term) = compare_and_swap(Op::DdlAbort, predicate, timeout)?; diff --git a/src/storage.rs b/src/storage.rs index 324a17602b..777acfffbb 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -34,16 +34,11 @@ macro_rules! define_clusterwide_spaces { pub #space_name_lower: #space_name_upper, } - $(#[$ClusterwideSpaceId_meta:meta])* - pub enum $ClusterwideSpaceId:ident { - #space_name_upper = #space_id, - } - $(#[$ClusterwideSpace_meta:meta])* pub enum $ClusterwideSpace:ident { $( $(#[$cw_field_meta:meta])* - $cw_space_var:ident = $cw_space_id_value:expr, $cw_space_name:expr => { + $cw_space_var:ident = $cw_space_id:expr, $cw_space_name:expr => { $_Clusterwide:ident :: $Clusterwide_field:ident; $(#[$space_struct_meta:meta])* @@ -69,47 +64,35 @@ macro_rules! define_clusterwide_spaces { } } - $(#[$ClusterwideSpaceId_meta])* - pub enum $ClusterwideSpaceId { - $( $cw_space_var = $cw_space_id_value, )+ - } - - impl From<$ClusterwideSpaceId> for $ClusterwideSpace { - fn from(space_id: $ClusterwideSpaceId) -> Self { - match space_id { - $( $ClusterwideSpaceId::$cw_space_var => Self::$cw_space_var, )+ - } - } - } - - impl From<$ClusterwideSpace> for $ClusterwideSpaceId { - fn from(space_name: $ClusterwideSpace) -> Self { - match space_name { - $( - $ClusterwideSpace::$cw_space_var => Self::$cw_space_var, - )+ + impl $ClusterwideSpace { + /// Id of the corrseponding system global space. + pub const fn id(&self) -> SpaceId { + match self { + $( Self::$cw_space_var => $cw_space_id, )+ } } - } - impl $ClusterwideSpaceId { - #[inline(always)] + /// Name of the corrseponding system global space. pub const fn name(&self) -> &'static str { match self { $( Self::$cw_space_var => $cw_space_name, )+ } } + + /// A slice of all possible variants of `Self`. + pub const fn all_spaces() -> &'static [Self] { + &[ $( Self::$cw_space_var, )+ ] + } } - impl TryFrom<SpaceId> for $ClusterwideSpaceId { - // TODO: conform to bureaucracy - type Error = (); + impl TryFrom<SpaceId> for $ClusterwideSpace { + type Error = SpaceId; #[inline(always)] - fn try_from(id: SpaceId) -> ::std::result::Result<$ClusterwideSpaceId, Self::Error> { + fn try_from(id: SpaceId) -> ::std::result::Result<$ClusterwideSpace, Self::Error> { match id { - $( $cw_space_id_value => Ok(Self::$cw_space_var), )+ - _ => Err(()), + $( $cw_space_id => Ok(Self::$cw_space_var), )+ + _ => Err(id), } } } @@ -176,7 +159,7 @@ macro_rules! define_clusterwide_spaces { impl TClusterwideSpace for $space_struct { const SPACE_NAME: &'static str = $cw_space_name; - const SPACE_ID: SpaceId = $cw_space_id_value; + const SPACE_ID: SpaceId = $cw_space_id; const INDEX_NAMES: &'static [&'static str] = &[ $index_name_pk, $( $index_name, )* @@ -186,7 +169,7 @@ macro_rules! define_clusterwide_spaces { } } -fn space_by_id(space_id: SpaceId) -> tarantool::Result<Space> { +fn space_by_id_unchecked(space_id: SpaceId) -> tarantool::Result<Space> { let space = unsafe { Space::from_id_unchecked(space_id) }; // TODO: maybe we should verify the space exists, but it's not a big deal // currently, because first of all tarantool api will just return a no such @@ -216,14 +199,7 @@ define_clusterwide_spaces! { pub #space_name_lower: #space_name_upper, } - /// An enumeration of system clusterwide spaces' ids. - #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq)] - pub enum ClusterwideSpaceId { - #space_name_upper = #space_id, - } - /// An enumeration of builtin cluster-wide spaces - // TODO: maybe rename ClusterwideSpaceName pub enum ClusterwideSpace { Instance = 515, "_pico_instance" => { Clusterwide::instances; @@ -333,7 +309,7 @@ impl Clusterwide { pub fn snapshot_data() -> tarantool::Result<SnapshotData> { let mut space_dumps = Self::internal_space_dumps()?; - let pico_space = space_by_name(&ClusterwideSpace::Space)?; + let pico_space = space_by_id_unchecked(ClusterwideSpace::Space.id())?; let iter = pico_space.select(IteratorType::All, &())?; for tuple in iter { let space_def: SpaceDef = tuple.decode()?; @@ -343,7 +319,7 @@ impl Clusterwide { space_dumps.push(Self::space_dump(&space_def.name)?); } - let pico_property = space_by_id(ClusterwideSpaceId::Property.value())?; + let pico_property = space_by_id_unchecked(ClusterwideSpace::Property.id())?; let tuple = pico_property.get(&[PropertyName::GlobalSchemaVersion.as_str()])?; let mut schema_version = 0; if let Some(tuple) = tuple { @@ -579,9 +555,9 @@ impl Clusterwide { space_id: SpaceId, index_id: IndexId, ) -> tarantool::Result<Rc<KeyDef>> { - static mut KEY_DEF: Option<HashMap<(ClusterwideSpaceId, IndexId), Rc<KeyDef>>> = None; + static mut KEY_DEF: Option<HashMap<(ClusterwideSpace, IndexId), Rc<KeyDef>>> = None; let key_defs = unsafe { KEY_DEF.get_or_insert_with(HashMap::new) }; - if let Ok(sys_space_id) = ClusterwideSpaceId::try_from(space_id) { + if let Ok(sys_space_id) = ClusterwideSpace::try_from(space_id) { let key_def = match key_defs.entry((sys_space_id, index_id)) { Entry::Occupied(o) => o.into_mut(), Entry::Vacant(v) => { @@ -607,9 +583,9 @@ impl Clusterwide { space_id: SpaceId, index_id: IndexId, ) -> tarantool::Result<Rc<KeyDef>> { - static mut KEY_DEF: Option<HashMap<(ClusterwideSpaceId, IndexId), Rc<KeyDef>>> = None; + static mut KEY_DEF: Option<HashMap<(ClusterwideSpace, IndexId), Rc<KeyDef>>> = None; let key_defs = unsafe { KEY_DEF.get_or_insert_with(HashMap::new) }; - if let Ok(sys_space_id) = ClusterwideSpaceId::try_from(space_id) { + if let Ok(sys_space_id) = ClusterwideSpace::try_from(space_id) { let key_def = match key_defs.entry((sys_space_id, index_id)) { Entry::Occupied(o) => o.into_mut(), Entry::Vacant(v) => { @@ -633,7 +609,7 @@ impl Clusterwide { space_id: SpaceId, tuple: &TupleBuffer, ) -> tarantool::Result<Tuple> { - let space = space_by_id(space_id)?; + let space = space_by_id_unchecked(space_id)?; let res = space.insert(tuple)?; Ok(res) } @@ -643,7 +619,7 @@ impl Clusterwide { space_id: SpaceId, tuple: &TupleBuffer, ) -> tarantool::Result<Tuple> { - let space = space_by_id(space_id)?; + let space = space_by_id_unchecked(space_id)?; let res = space.replace(tuple)?; Ok(res) } @@ -654,7 +630,7 @@ impl Clusterwide { key: &TupleBuffer, ops: &[TupleBuffer], ) -> tarantool::Result<Option<Tuple>> { - let space = space_by_id(space_id)?; + let space = space_by_id_unchecked(space_id)?; let res = space.update(key, ops)?; Ok(res) } @@ -664,7 +640,7 @@ impl Clusterwide { space_id: SpaceId, key: &TupleBuffer, ) -> tarantool::Result<Option<Tuple>> { - let space = space_by_id(space_id)?; + let space = space_by_id_unchecked(space_id)?; let res = space.delete(key)?; Ok(res) } @@ -705,17 +681,10 @@ pub trait TClusterwideSpace { const INDEX_NAMES: &'static [&'static str]; } -impl ClusterwideSpaceId { - #[inline(always)] - pub const fn value(&self) -> SpaceId { - *self as _ - } -} - -impl From<ClusterwideSpaceId> for SpaceId { +impl From<ClusterwideSpace> for SpaceId { #[inline(always)] - fn from(id: ClusterwideSpaceId) -> SpaceId { - id as _ + fn from(space: ClusterwideSpace) -> SpaceId { + space.id() } } diff --git a/src/traft/node.rs b/src/traft/node.rs index a63209007e..d60c9ae8ac 100644 --- a/src/traft/node.rs +++ b/src/traft/node.rs @@ -20,7 +20,7 @@ use crate::storage::ddl_meta_drop_space; use crate::storage::SnapshotData; use crate::storage::{ddl_abort_on_master, ddl_meta_space_update_operable}; use crate::storage::{local_schema_version, set_local_schema_version}; -use crate::storage::{Clusterwide, ClusterwideSpaceId, PropertyName}; +use crate::storage::{Clusterwide, ClusterwideSpace, PropertyName}; use crate::stringify_cfunc; use crate::sync; use crate::tlog; @@ -56,7 +56,6 @@ use ::tarantool::index::FieldType as IFT; use ::tarantool::index::Part; use ::tarantool::proc; use ::tarantool::space::FieldType as SFT; -use ::tarantool::space::SpaceId; use ::tarantool::time::Instant; use ::tarantool::tlua; use ::tarantool::transaction::transaction; @@ -640,11 +639,11 @@ impl NodeImpl { match &op { Op::Dml(op) => { let space = op.space(); - if space == ClusterwideSpaceId::Property as SpaceId - || space == ClusterwideSpaceId::Replicaset as SpaceId + if space == ClusterwideSpace::Property.id() + || space == ClusterwideSpace::Replicaset.id() { *wake_governor = true; - } else if space == ClusterwideSpaceId::Instance as SpaceId { + } else if space == ClusterwideSpace::Instance.id() { *wake_governor = true; let instance = match op { Dml::Insert { tuple, .. } => Some(tuple), -- GitLab