Skip to content

Commit

Permalink
Merge pull request #590 from drmingdrmer/50-rm-missing-node-info
Browse files Browse the repository at this point in the history 
Change: remove error MissingNodeInfo
  • Loading branch information
@drmingdrmer
drmingdrmer authored Nov 2, 2022
2 parents 869bf64 + a12fd8e commit 5928fe6
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 90 deletions.
21 changes: 2 additions & 19 deletions openraft/src/core/raft_core.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -477,14 +477,7 @@ impl<C: RaftTypeConfig, N: RaftNetworkFactory<C>, S: RaftStorage<C>> RaftCore<C,
}

let curr = &self.engine.state.membership_state.effective.membership;
let res = curr.add_learner(target, node);
let new_membership = match res {
Ok(x) => x,
Err(e) => {
let _ = tx.send(Err(AddLearnerError::MissingNodeInfo(e)));
return Ok(());
}
};
let new_membership = curr.add_learner(target, node);

tracing::debug!(?new_membership, "new_membership with added learner: {}", target);

Expand Down Expand Up @@ -538,17 +531,7 @@ impl<C: RaftTypeConfig, N: RaftNetworkFactory<C>, S: RaftStorage<C>> RaftCore<C,
let old_members = mem.voter_ids().collect::<BTreeSet<_>>();
let only_in_new = members.difference(&old_members);

let new_config = {
let res = curr.next_safe(members.clone(), turn_to_learner);
match res {
Ok(x) => x,
Err(e) => {
let change_err = ChangeMembershipError::MissingNodeInfo(e);
let _ = tx.send(Err(ClientWriteError::ChangeMembershipError(change_err)));
return Ok(());
}
}
};
let new_config = curr.next_safe(members.clone(), turn_to_learner);

tracing::debug!(?new_config, "new_config");

Expand Down
19 changes: 1 addition & 18 deletions openraft/src/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,6 @@ pub enum ChangeMembershipError<NID: NodeId> {

#[error(transparent)]
LearnerIsLagging(#[from] LearnerIsLagging<NID>),

#[error(transparent)]
MissingNodeInfo(#[from] MissingNodeInfo<NID>),
}

#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
Expand All @@ -162,9 +159,7 @@ where
#[error(transparent)]
ForwardToLeader(#[from] ForwardToLeader<NID, N>),

#[error(transparent)]
MissingNodeInfo(#[from] MissingNodeInfo<NID>),

// TODO: do we really need this error? An app may check an target node if it wants to.
#[error(transparent)]
NetworkError(#[from] NetworkError),

Expand Down Expand Up @@ -204,9 +199,6 @@ where
#[error(transparent)]
NotAMembershipEntry(#[from] NotAMembershipEntry),

#[error(transparent)]
MissingNodeInfo(#[from] MissingNodeInfo<NID>),

#[error(transparent)]
Fatal(#[from] Fatal<NID>),
}
Expand Down Expand Up @@ -463,15 +455,6 @@ pub struct NotAllowed<NID: NodeId> {
pub vote: Vote<NID>,
}

#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize), serde(bound = ""))]
#[error("node {node_id} {reason}")]
// TODO: remove it
pub struct MissingNodeInfo<NID: NodeId> {
pub node_id: NID,
pub reason: String,
}

#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize), serde(bound = ""))]
#[error("node {node_id} has to be a member. membership:{membership:?}")]
Expand Down
42 changes: 19 additions & 23 deletions openraft/src/membership/membership.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use std::collections::BTreeSet;

use maplit::btreemap;

use crate::error::MissingNodeInfo;
use crate::membership::NodeRole;
use crate::node::Node;
use crate::quorum::AsJoint;
Expand Down Expand Up @@ -83,8 +82,7 @@ where
fn from(b: BTreeMap<NID, N>) -> Self {
let member_ids = b.keys().cloned().collect::<BTreeSet<NID>>();

// Safe unwrap: every node-id in `member_ids` present in `b`.
Membership::with_nodes(vec![member_ids], b).unwrap()
Membership::with_nodes(vec![member_ids], b)
}
}

Expand Down Expand Up @@ -159,25 +157,26 @@ where
/// Create a new Membership of multiple configs and optional node infos.
///
/// The node infos `nodes` can be:
/// - a simple `()`, if there are no non-member nodes and no node infos are provided,
/// - `BTreeSet<NodeId>` to specify additional learner node ids without node infos provided,
/// - `BTreeMap<NodeId, Node>` or `BTreeMap<NodeId, Option<Node>>` to specify learner nodes with node infos. Node
/// ids not in `configs` are learner node ids. In this case, every node id in `configs` has to present in `nodes`
/// or an error will be returned.
pub(crate) fn with_nodes<T>(configs: Vec<BTreeSet<NID>>, nodes: T) -> Result<Self, MissingNodeInfo<NID>>
/// - a simple `()`, if there are no non-voter(learner) nodes,
/// - `BTreeSet<NodeId>` provides learner node ids whose `Node` data are `Node::default()`,
/// - `BTreeMap<NodeId, Node>` provides nodes for every node id. Node ids that are not in `configs` are learners.
///
/// Every node id in `configs` has to present in `nodes`.
pub(crate) fn with_nodes<T>(configs: Vec<BTreeSet<NID>>, nodes: T) -> Self
where T: IntoOptionNodes<NID, N> {
let nodes = nodes.into_option_nodes();

for voter_id in configs.as_joint().ids() {
if !nodes.contains_key(&voter_id) {
return Err(MissingNodeInfo {
node_id: voter_id,
reason: format!("is not in cluster: {:?}", nodes.keys().cloned().collect::<Vec<_>>()),
});
if cfg!(debug_assertions) {
for voter_id in configs.as_joint().ids() {
assert!(
nodes.contains_key(&voter_id),
"nodes has to contain voter id {}",
voter_id
);
}
}

Ok(Membership { configs, nodes })
Membership { configs, nodes }
}

/// Extends nodes btreemap with another.
Expand All @@ -202,14 +201,12 @@ where
self.configs.len() > 1
}

pub(crate) fn add_learner(&self, node_id: NID, node: N) -> Result<Self, MissingNodeInfo<NID>> {
pub(crate) fn add_learner(&self, node_id: NID, node: N) -> Self {
let configs = self.configs.clone();

let nodes = Self::extend_nodes(self.nodes.clone(), &btreemap! {node_id=>node});

let m = Self::with_nodes(configs, nodes)?;

Ok(m)
Self::with_nodes(configs, nodes)
}
}

Expand Down Expand Up @@ -299,7 +296,7 @@ where
/// curr = next;
/// }
/// ```
pub(crate) fn next_safe<T>(&self, goal: T, turn_to_learner: bool) -> Result<Self, MissingNodeInfo<NID>>
pub(crate) fn next_safe<T>(&self, goal: T, turn_to_learner: bool) -> Self
where T: IntoOptionNodes<NID, N> {
let goal = goal.into_option_nodes();

Expand All @@ -318,8 +315,7 @@ where
}
};

let m = Membership::with_nodes(config, nodes)?;
Ok(m)
Membership::with_nodes(config, nodes)
}

/// Build a QuorumSet from current joint config
Expand Down
58 changes: 28 additions & 30 deletions openraft/src/membership/membership_test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use std::fmt::Formatter;
use maplit::btreemap;
use maplit::btreeset;

use crate::error::MissingNodeInfo;
use crate::Membership;
use crate::MessageSummary;

Expand Down Expand Up @@ -47,7 +46,7 @@ fn test_membership_summary() -> anyhow::Result<()> {
3=>node("127.0.0.3", "k3"),
4=>node("127.0.0.4", "k4"),

})?;
});
assert_eq!(
"members:[{1:{TestNode { addr: \"127.0.0.1\", data: {\"k1\": \"k1\"} }},2:{TestNode { addr: \"127.0.0.2\", data: {\"k2\": \"k2\"} }}},{3:{TestNode { addr: \"127.0.0.3\", data: {\"k3\": \"k3\"} }}}],learners:[4:{TestNode { addr: \"127.0.0.4\", data: {\"k4\": \"k4\"} }}]",
m.summary()
Expand Down Expand Up @@ -90,7 +89,7 @@ fn test_membership_with_learners() -> anyhow::Result<()> {
// test multi membership with learners
{
let m1_2 = Membership::<u64, ()>::new(vec![btreeset! {1}], Some(btreeset! {2}));
let m1_23 = m1_2.add_learner(3, ())?;
let m1_23 = m1_2.add_learner(3, ());

// test learner and membership
assert_eq!(vec![1], m1_2.voter_ids().collect::<Vec<_>>());
Expand All @@ -101,12 +100,12 @@ fn test_membership_with_learners() -> anyhow::Result<()> {

// Adding a member as learner has no effect:

let m = m1_23.add_learner(1, ())?;
let m = m1_23.add_learner(1, ());
assert_eq!(vec![1], m.voter_ids().collect::<Vec<_>>());

// Adding a existent learner has no effect:

let m = m1_23.add_learner(3, ())?;
let m = m1_23.add_learner(3, ());
assert_eq!(vec![1], m.voter_ids().collect::<Vec<_>>());
assert_eq!(btreeset! {2,3}, m.learner_ids().collect());
}
Expand All @@ -131,21 +130,21 @@ fn test_membership_add_learner() -> anyhow::Result<()> {
let m_1_2 = Membership::<u64, TestNode>::with_nodes(
vec![btreeset! {1}, btreeset! {2}],
btreemap! {1=>node("1"), 2=>node("2")},
)?;
);

// Add learner that presents in old cluster has no effect.

let res = m_1_2.add_learner(1, node("3"))?;
let res = m_1_2.add_learner(1, node("3"));
assert_eq!(m_1_2, res);

// Success to add a learner

let m_1_2_3 = m_1_2.add_learner(3, node("3"))?;
let m_1_2_3 = m_1_2.add_learner(3, node("3"));
assert_eq!(
Membership::<u64, TestNode>::with_nodes(
vec![btreeset! {1}, btreeset! {2}],
btreemap! {1=>node("1"), 2=>node("2"), 3=>node("3")}
)?,
),
m_1_2_3
);

Expand Down Expand Up @@ -181,31 +180,30 @@ fn test_membership_with_nodes() -> anyhow::Result<()> {
let node = TestNode::default;
let with_nodes = |nodes| Membership::<u64, TestNode>::with_nodes(vec![btreeset! {1}, btreeset! {2}], nodes);

let res = with_nodes(btreemap! {1=>node(), 2=>node()})?;
let res = with_nodes(btreemap! {1=>node(), 2=>node()});
assert_eq!(
btreemap! {1=>node(), 2=>node()},
res.nodes().map(|(nid, n)| (*nid, n.clone())).collect::<BTreeMap<_, _>>()
);

let res = with_nodes(btreemap! {1=>node(), 2=>node(),3=>node()})?;
let res = with_nodes(btreemap! {1=>node(), 2=>node(),3=>node()});
assert_eq!(
btreemap! {1=>node(), 2=>node(), 3=>node()},
res.nodes().map(|(nid, n)| (*nid, n.clone())).collect::<BTreeMap<_, _>>()
);

// errors:
let res = with_nodes(btreemap! {1=>node()});
assert_eq!(
Err(MissingNodeInfo {
node_id: 2,
reason: "is not in cluster: [1]".to_string(),
}),
res
);

Ok(())
}

// TODO: rename
#[test]
#[should_panic]
fn test_membership_with_nodes_panic() {
// Panic if debug_assertions is enabled:
// voter ids set is {1,2}, while the nodes set is {1}
Membership::<u64, TestNode>::with_nodes(vec![btreeset! {1}, btreeset! {2}], btreemap! {1=>TestNode::default()});
}

#[test]
fn test_membership_next_safe() -> anyhow::Result<()> {
let c1 = || btreeset! {1,2,3};
Expand All @@ -217,19 +215,19 @@ fn test_membership_next_safe() -> anyhow::Result<()> {
let m12 = Membership::<u64, ()>::new(vec![c1(), c2()], None);
let m23 = Membership::<u64, ()>::new(vec![c2(), c3()], None);

assert_eq!(m1, m1.next_safe(c1(), false)?);
assert_eq!(m12, m1.next_safe(c2(), false)?);
assert_eq!(m1, m12.next_safe(c1(), false)?);
assert_eq!(m2, m12.next_safe(c2(), false)?);
assert_eq!(m23, m12.next_safe(c3(), false)?);
assert_eq!(m1, m1.next_safe(c1(), false));
assert_eq!(m12, m1.next_safe(c2(), false));
assert_eq!(m1, m12.next_safe(c1(), false));
assert_eq!(m2, m12.next_safe(c2(), false));
assert_eq!(m23, m12.next_safe(c3(), false));

// Turn removed members to learners

let old_learners = || btreeset! {1, 2};
let learners = || btreeset! {1, 2, 3, 4, 5};
let m23_with_learners_old = Membership::<u64, ()>::new(vec![c2(), c3()], Some(old_learners()));
let m23_with_learners_new = Membership::<u64, ()>::new(vec![c3()], Some(learners()));
assert_eq!(m23_with_learners_new, m23_with_learners_old.next_safe(c3(), true)?);
assert_eq!(m23_with_learners_new, m23_with_learners_old.next_safe(c3(), true));

Ok(())
}
Expand All @@ -244,19 +242,19 @@ fn test_membership_next_safe_with_nodes() -> anyhow::Result<()> {
let c1 = || btreeset! {1};
let c2 = || btreeset! {2};

let initial = Membership::<u64, TestNode>::with_nodes(vec![c1(), c2()], btreemap! {1=>node("1"), 2=>node("2")})?;
let initial = Membership::<u64, TestNode>::with_nodes(vec![c1(), c2()], btreemap! {1=>node("1"), 2=>node("2")});

// joint [{2}, {1,2}]

let res = initial.next_safe(btreeset! {1,2}, false)?;
let res = initial.next_safe(btreeset! {1,2}, false);
assert_eq!(
btreemap! {1=>node("1"), 2=>node("2")},
res.nodes().map(|(nid, n)| (*nid, n.clone())).collect::<BTreeMap<_, _>>()
);

// Removed to learner

let res = initial.next_safe(btreeset! {1}, true)?;
let res = initial.next_safe(btreeset! {1}, true);
assert_eq!(
btreemap! {1=>node("1"), 2=>node("2")},
res.nodes().map(|(nid, n)| (*nid, n.clone())).collect::<BTreeMap<_, _>>()
Expand Down

0 comments on commit 5928fe6

Please sign in to comment.