Skip to content

Commit

Permalink
Change: remove error NodeNotFound
Browse files Browse the repository at this point in the history
A node is stored in `Membership` thus it should always be found.
Otherwise it is a bug of openraft.
In either case, there is no need for an application to deal with
`RPCError::NodeNotFound` error.

An application that needs such an error should define it as an
application error.

- Migration guide:
  if you do have been using it, you could just replace `NodeNotFound` with `NetworkError`.

- Fix: #623
  • Loading branch information
drmingdrmer committed Dec 28, 2022
1 parent 3ab040b commit 9311631
Show file tree
Hide file tree
Showing 5 changed files with 5 additions and 64 deletions.
8 changes: 1 addition & 7 deletions examples/raft-kv-memstore/tests/cluster/test_cluster.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,10 @@ use std::time::Duration;

use maplit::btreemap;
use maplit::btreeset;
use openraft::error::NodeNotFound;
use openraft::AnyError;
use openraft::BasicNode;
use raft_kv_memstore::client::ExampleClient;
use raft_kv_memstore::start_example_raft_node;
use raft_kv_memstore::store::ExampleRequest;
use raft_kv_memstore::ExampleNodeId;
use tokio::runtime::Runtime;
use tracing_subscriber::EnvFilter;

Expand All @@ -36,10 +33,7 @@ async fn test_cluster() -> anyhow::Result<()> {
2 => "127.0.0.1:21002".to_string(),
3 => "127.0.0.1:21003".to_string(),
_ => {
return Err(NodeNotFound::<ExampleNodeId> {
node_id,
source: AnyError::error("node not found"),
});
return Err(anyhow::anyhow!("node {} not found", node_id));
}
};
Ok(addr)
Expand Down
33 changes: 0 additions & 33 deletions examples/raft-kv-rocksdb/src/network/raft_network_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use async_trait::async_trait;
use openraft::error::AppendEntriesError;
use openraft::error::InstallSnapshotError;
use openraft::error::NetworkError;
use openraft::error::NodeNotFound;
use openraft::error::RPCError;
use openraft::error::RemoteError;
use openraft::error::VoteError;
Expand All @@ -19,7 +18,6 @@ use openraft::AnyError;
use openraft::RaftNetwork;
use openraft::RaftNetworkFactory;
use serde::de::DeserializeOwned;
use serde::Serialize;
use toy_rpc::pubsub::AckModeNone;
use toy_rpc::Client;

Expand All @@ -30,37 +28,6 @@ use crate::ExampleTypeConfig;

pub struct ExampleNetwork {}

impl ExampleNetwork {
pub async fn send_rpc<Req, Resp, Err>(
&self,
target: ExampleNodeId,
target_node: Option<&ExampleNode>,
uri: &str,
req: Req,
) -> Result<Resp, RPCError<ExampleNodeId, ExampleNode, Err>>
where
Req: Serialize,
Err: std::error::Error + DeserializeOwned,
Resp: DeserializeOwned,
{
let addr = target_node.map(|x| &x.rpc_addr).ok_or_else(|| {
RPCError::NodeNotFound(NodeNotFound {
node_id: target,
source: AnyError::default(),
})
})?;

let url = format!("http://{}/{}", addr, uri);
let client = reqwest::Client::new();

let resp = client.post(url).json(&req).send().await.map_err(|e| RPCError::Network(NetworkError::new(&e)))?;

let res: Result<Resp, Err> = resp.json().await.map_err(|e| RPCError::Network(NetworkError::new(&e)))?;

res.map_err(|e| RPCError::RemoteError(RemoteError::new(target, e)))
}
}

// NOTE: This could be implemented also on `Arc<ExampleNetwork>`, but since it's empty, implemented directly.
#[async_trait]
impl RaftNetworkFactory<ExampleTypeConfig> for ExampleNetwork {
Expand Down
14 changes: 0 additions & 14 deletions openraft/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,9 +283,6 @@ where
#[error(transparent)]
StorageError(#[from] StorageError<NID>),

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

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

Expand All @@ -303,9 +300,6 @@ where
serde(bound = "T:serde::Serialize + for <'d> serde::Deserialize<'d>")
)]
pub enum RPCError<NID: NodeId, N: Node, T: Error> {
#[error(transparent)]
NodeNotFound(#[from] NodeNotFound<NID>),

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

Expand Down Expand Up @@ -477,14 +471,6 @@ pub struct NotAMembershipEntry {}
#[error("new membership can not be empty")]
pub struct EmptyMembership {}

#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize), serde(bound = ""))]
#[error("node not found: {node_id}, source: {source}")]
pub struct NodeNotFound<NID: NodeId> {
pub node_id: NID,
pub source: AnyError,
}

#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
#[error("infallible")]
Expand Down
4 changes: 0 additions & 4 deletions openraft/src/replication/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,6 @@ impl<C: RaftTypeConfig, N: RaftNetworkFactory<C>, S: RaftStorage<C>> Replication
let _ = self.tx_raft_core.send(RaftMsg::ReplicationFatal);
return;
}
ReplicationError::NodeNotFound(err) => {
unreachable!("programming bug: {}", err)
}
ReplicationError::Timeout { .. } => {
// nothing to do
}
Expand Down Expand Up @@ -306,7 +303,6 @@ impl<C: RaftTypeConfig, N: RaftNetworkFactory<C>, S: RaftStorage<C>> Replication
tracing::warn!(error=%err, "error sending AppendEntries RPC to target");

let repl_err = match err {
RPCError::NodeNotFound(e) => ReplicationError::NodeNotFound(e),
RPCError::Timeout(e) => {
let _ = self.tx_raft_core.send(RaftMsg::UpdateReplicationProgress {
target: self.target,
Expand Down
10 changes: 4 additions & 6 deletions openraft/tests/fixtures/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ use openraft::error::CheckIsLeaderError;
use openraft::error::ClientWriteError;
use openraft::error::InstallSnapshotError;
use openraft::error::NetworkError;
use openraft::error::NodeNotFound;
use openraft::error::RPCError;
use openraft::error::RemoteError;
use openraft::error::VoteError;
Expand Down Expand Up @@ -423,12 +422,11 @@ where
}

#[tracing::instrument(level = "debug", skip(self))]
pub fn get_raft_handle(&self, node_id: &C::NodeId) -> std::result::Result<MemRaft<C, S>, NodeNotFound<C::NodeId>> {
pub fn get_raft_handle(&self, node_id: &C::NodeId) -> std::result::Result<MemRaft<C, S>, NetworkError> {
let rt = self.routing_table.lock().unwrap();
let raft_and_sto = rt.get(node_id).ok_or_else(|| NodeNotFound {
node_id: *node_id,
source: AnyError::error(""),
})?;
let raft_and_sto = rt
.get(node_id)
.ok_or_else(|| NetworkError::new(&AnyError::error(format!("node {} not found", *node_id))))?;
let r = raft_and_sto.clone().0;
Ok(r)
}
Expand Down

0 comments on commit 9311631

Please sign in to comment.