From 259ff4ac0d8154a67764b099f941ea7b408b5641 Mon Sep 17 00:00:00 2001
From: Georgy Moshkin <gmoshkin@picodata.io>
Date: Thu, 26 Sep 2024 15:57:02 +0300
Subject: [PATCH] refactor: don't use `storage` in `update_instance`
---
src/instance.rs | 25 ++++++++++++++-----------
src/rpc/update_instance.rs | 18 ++++++++----------
src/util.rs | 4 ++++
3 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/src/instance.rs b/src/instance.rs
index ceff216335..4c867175db 100644
--- a/src/instance.rs
+++ b/src/instance.rs
@@ -337,6 +337,7 @@ mod tests {
let storage = Clusterwide::for_tests();
add_tier(&storage, DEFAULT_TIER, 1, true).unwrap();
let instance = add_instance(&storage, 1, "i1", "r1", &State::new(Online, 1)).unwrap();
+ let existing_fds = HashSet::new();
//
// Current state incarnation is allowed to go down,
@@ -344,7 +345,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_current_state(State::new(Offline, 0));
- let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
+ let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("current_state", State::new(Offline, 0)).unwrap();
@@ -359,7 +360,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_current_state(State::new(Offline, 0));
- let dml = update_instance(&instance, &req, &storage).unwrap();
+ let dml = update_instance(&instance, &req, &existing_fds).unwrap();
assert_eq!(dml, None);
//
@@ -367,7 +368,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_target_state(Offline);
- let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
+ let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("target_state", State::new(Offline, 0)).unwrap();
@@ -382,7 +383,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_target_state(Online);
- let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
+ let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("target_state", State::new(Online, 1)).unwrap();
@@ -397,7 +398,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_target_state(Online);
- let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
+ let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("target_state", State::new(Online, 2)).unwrap();
@@ -412,7 +413,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_target_state(Expelled);
- let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
+ let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("target_state", State::new(Expelled, 0)).unwrap();
@@ -427,7 +428,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_current_state(State::new(Expelled, 69));
- let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
+ let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("current_state", State::new(Expelled, 69)).unwrap();
@@ -442,7 +443,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_target_state(Online);
- let e = update_instance(&instance, &req, &storage).unwrap_err();
+ let e = update_instance(&instance, &req, &existing_fds).unwrap_err();
assert_eq!(e.to_string(), "cannot update expelled instance \"i1\"");
}
@@ -516,6 +517,7 @@ mod tests {
//
let instance1 = build_instance(Some(&InstanceId::from("i1")), None, &faildoms! {planet: Earth}, &storage, DEFAULT_TIER).unwrap();
storage.instances.put(&instance1).unwrap();
+ let existing_fds = storage.instances.failure_domain_names().unwrap();
assert_eq!(instance1.failure_domain, faildoms! {planet: Earth});
assert_eq!(instance1.replicaset_id, "r1");
@@ -524,7 +526,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance1.instance_id.clone(), "".into())
.with_failure_domain(faildoms! {owner: Ivan});
- let e = update_instance(&instance1, &req, &storage).unwrap_err();
+ let e = update_instance(&instance1, &req, &existing_fds).unwrap_err();
assert_eq!(e.to_string(), "missing failure domain names: PLANET");
//
@@ -533,7 +535,7 @@ mod tests {
let fd = faildoms! {planet: Mars, owner: Ivan};
let req = rpc::update_instance::Request::new(instance1.instance_id.clone(), "".into())
.with_failure_domain(fd.clone());
- let (dml, do_bump) = update_instance(&instance1, &req, &storage).unwrap().unwrap();
+ let (dml, do_bump) = update_instance(&instance1, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("failure_domain", fd).unwrap();
@@ -556,6 +558,7 @@ mod tests {
#[rustfmt::skip]
let instance2 = build_instance(Some(&InstanceId::from("i2")), None, &fd, &storage, DEFAULT_TIER).unwrap();
storage.instances.put(&instance2).unwrap();
+ let existing_fds = storage.instances.failure_domain_names().unwrap();
assert_eq!(instance2.failure_domain, fd);
// doesn't fit into r1
assert_eq!(instance2.replicaset_id, "r2");
@@ -566,7 +569,7 @@ mod tests {
let fd = faildoms! {planet: Earth, owner: Mike};
let req = rpc::update_instance::Request::new(instance2.instance_id.clone(), "".into())
.with_failure_domain(fd.clone());
- let (dml, do_bump) = update_instance(&instance2, &req, &storage).unwrap().unwrap();
+ let (dml, do_bump) = update_instance(&instance2, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("failure_domain", fd).unwrap();
diff --git a/src/rpc/update_instance.rs b/src/rpc/update_instance.rs
index 708a2ff7fc..a9704f0a81 100644
--- a/src/rpc/update_instance.rs
+++ b/src/rpc/update_instance.rs
@@ -1,5 +1,3 @@
-use std::time::Duration;
-
use crate::cas;
use crate::column_name;
use crate::failure_domain::FailureDomain;
@@ -9,12 +7,14 @@ use crate::instance::StateVariant::*;
use crate::instance::{Instance, InstanceId};
use crate::replicaset::Replicaset;
use crate::schema::ADMIN_ID;
-use crate::storage::Clusterwide;
use crate::storage::ClusterwideTable;
use crate::tier::Tier;
use crate::traft::op::{Dml, Op};
use crate::traft::Result;
use crate::traft::{error::Error, node};
+use crate::util::Uppercase;
+use std::collections::HashSet;
+use std::time::Duration;
use tarantool::fiber;
use tarantool::space::UpdateOps;
@@ -125,7 +125,9 @@ pub fn handle_update_instance_request_and_wait(req: Request, timeout: Duration)
return Err(Error::NoSuchReplicaset { id: replicaset_id.to_string(), id_is_uuid: false });
};
- let dml = update_instance(&instance, &req, storage)?;
+ let existing_fds = storage.instances.failure_domain_names()?;
+
+ let dml = update_instance(&instance, &req, &existing_fds)?;
let Some((dml, version_bump_needed)) = dml else {
// No point in proposing an operation which doesn't change anything.
// Note: if the request tried setting target state Online while it
@@ -196,7 +198,7 @@ pub fn handle_update_instance_request_and_wait(req: Request, timeout: Duration)
pub fn update_instance(
instance: &Instance,
req: &Request,
- storage: &Clusterwide,
+ existing_fds: &HashSet<Uppercase>,
) -> Result<Option<(Dml, bool)>> {
if instance.current_state.variant == Expelled
&& !matches!(
@@ -215,11 +217,7 @@ pub fn update_instance(
let mut ops = UpdateOps::new();
if let Some(fd) = req.failure_domain.as_ref() {
- let existing_fds = storage
- .instances
- .failure_domain_names()
- .expect("storage should not fail");
- fd.check(&existing_fds)?;
+ fd.check(existing_fds)?;
if fd != &instance.failure_domain {
ops.assign(column_name!(Instance, failure_domain), fd)?;
}
diff --git a/src/util.rs b/src/util.rs
index a12c556e68..a46be3600a 100644
--- a/src/util.rs
+++ b/src/util.rs
@@ -171,6 +171,10 @@ macro_rules! define_string_newtype {
////////////////////////////////////////////////////////////////////////////////
/// A wrapper around `String` that garantees the string is uppercase by
/// converting it to uppercase (if needed) on construction.
+// TODO: this is too much boilerplate for such a simple feature. We should
+// simplify and get rid of this struct, just use `String`, make sure it's
+// uppercase after we get it from the user (arguments, config, env, etc.)
+// and sprinkle `debug_assert` in places where it matters (when comparing values)
#[derive(Default, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, serde::Serialize)]
pub struct Uppercase(String);
--
GitLab