From a06ff88de86c0eb7f964c1a2d3a965147ed82d2c Mon Sep 17 00:00:00 2001
From: Yaroslav Dynnikov <yaroslav.dynnikov@gmail.com>
Date: Mon, 30 May 2022 18:32:14 +0300
Subject: [PATCH] fix: remove unique index on peer_address
The `peer_address` parameter is an inbound address used for
communication with the peer. It shouldn't be confused with the listen
address. The persisted `peer_address` may become obsolete due to
circumstances beyond picodata control (e.g. DNS or IP changes). Thus
there's no point in its prior validation, including the uniqueness
check.
There's also no such task as getting peer by peer_address.
To sum up, an index over `peer_address` is useless. It only creates
problems and causes panics.
Close https://git.picodata.io/picodata/picodata/picodata/-/issues/88
---
src/traft/mod.rs | 6 ++++++
src/traft/storage.rs | 39 ++++++++++++++++-----------------------
2 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/src/traft/mod.rs b/src/traft/mod.rs
index 375a793102..dd108472da 100644
--- a/src/traft/mod.rs
+++ b/src/traft/mod.rs
@@ -65,8 +65,14 @@ pub enum Op {
/// Serializable struct representing a member of the raft group.
#[derive(Clone, Debug, Default, Serialize, Deserialize, PartialEq)]
pub struct Peer {
+ /// Used for identifying raft nodes.
+ /// Must be unique in the raft group.
pub raft_id: u64,
+ /// Inbound address used for communication with the node.
+ /// Not to be confused with listen address.
pub peer_address: String,
+ /// Reflects the role of the node in the raft group.
+ /// Non-voters are also called learners in terms of raft.
pub voter: bool,
pub instance_id: String,
pub replicaset_id: String,
diff --git a/src/traft/storage.rs b/src/traft/storage.rs
index b935ec986c..16d84c995f 100644
--- a/src/traft/storage.rs
+++ b/src/traft/storage.rs
@@ -97,11 +97,6 @@ impl Storage {
parts = {{'replicaset_id'}, {'commit_index'}},
unique = false,
})
- box.space.raft_group:create_index('peer_address', {
- if_not_exists = true,
- parts = {{'peer_address'}},
- unique = true,
- })
"#,
);
}
@@ -558,24 +553,19 @@ inventory::submit!(crate::InnerTest {
)
);
- assert_err!(
- Storage::persist_peer(&traft::Peer {
- raft_id: 99,
- peer_address: "addr:1".into(),
+ {
+ // Ensure traft storage doesn't impose restrictions
+ // on peer_address uniqueness.
+ let peer = |id: u64, addr: &str| traft::Peer {
+ raft_id: id,
+ instance_id: format!("i{id}"),
+ peer_address: addr.into(),
..Default::default()
- }),
- concat!(
- "unknown error",
- " Tarantool error:",
- " TupleFound: Duplicate key exists",
- " in unique index \"peer_address\"",
- " in space \"raft_group\"",
- " with old tuple",
- " - [1, \"addr:1\", true, \"i1\", \"r1\", \"i1-uuid\", \"r1-uuid\", 1]",
- " and new tuple",
- " - [99, \"addr:1\", false, \"\", \"\", \"\", \"\", 0]"
- )
- );
+ };
+
+ Storage::persist_peer(&peer(10, "addr:collision")).unwrap();
+ Storage::persist_peer(&peer(11, "addr:collision")).unwrap();
+ }
let peer_by_raft_id = |id: u64| Storage::peer_by_raft_id(id).unwrap().unwrap();
{
@@ -594,6 +584,10 @@ inventory::submit!(crate::InnerTest {
assert_eq!(peer_by_instance_id("i3").peer_address, "addr:3");
assert_eq!(peer_by_instance_id("i4").peer_address, "addr:4");
assert_eq!(peer_by_instance_id("i5").peer_address, "addr:5");
+ assert_eq!(
+ peer_by_instance_id("i10").peer_address,
+ peer_by_instance_id("i11").peer_address
+ );
assert_eq!(Storage::peer_by_instance_id("i6"), Ok(None));
}
@@ -642,7 +636,6 @@ inventory::submit!(crate::InnerTest {
)
);
- raft_group.index("peer_address").unwrap().drop().unwrap();
raft_group.primary_key().drop().unwrap();
assert_err!(
--
GitLab