Do we actually need to send that many typed errors over the wire?

[xDarksome]

The current RaftNetwork interface requires to serialize/deserialize these error variants (listed them as a single enum for simplicity):

enum Error {
    // Fatal
    StorageError(StorageError<NID>),
    Panicked,
    Stopped,
    
    Timeout(Timeout<NID>),
    Network(NetworkError),
    
    // Only for install_snapshost op
    InstallSnapshotError(InstallSnapshotError)
}

This forces the driver network API adapter (besides the driven adapter) to also return these types. Although our application only cares about the following:

enum Error {
    Transient(anyhow::Error), // retry 
    Fatal(anyhow::Error) // reinitialize `Raft`
}

I looked through the openraft codebase and it seems to me that those errors currently aren't required at all for RPC error handling. All of them are just being converted into String for summary / logging.

It looks like the code that calls RaftNetwork functions is only concerned about whether there is an error and not the specifics of the error.

Does it make sense / is it possible to have the following RaftNetwork API?

pub trait RaftNetwork<C>: Send + Sync + 'static
where C: RaftTypeConfig
{
    type Err; // maybe some trait bounds here

    async fn send_append_entries(
        &mut self,
        rpc: AppendEntriesRequest<C>,
    ) -> Result<AppendEntriesResponse<C::NodeId>,  Self::Err>;

    async fn send_install_snapshot(
        &mut self,
        rpc: InstallSnapshotRequest<C>,
    ) -> Result<
        InstallSnapshotResponse<C::NodeId>,
        RPCError<C::NodeId, C::Node, Self::Err>,
    >;

    async fn send_vote(
        &mut self,
        rpc: VoteRequest<C::NodeId>,
    ) -> Result<VoteResponse<C::NodeId>, Self::Err>;
}

[schreter]

+1 for making error types configurable, probably via RaftTypeConfig for general ones and maybe associated types for particular traits, as suggested here.

@xDarksome The error you mentioned would then probably have a trait bound RpcError with method is_fatal() or the like to distinguish those two possibilities, right?

We learned the hard way in our project that error handling requires a bit more careful design - we have currently too many error enums, also deeply nested, and their value is practically zero. But they take many resources unnecessarily (especially making return types large). Therefore, I would suggest to collect some proposals for a holistic approach and only decide on the best one afterwards.

[xDarksome]

For my particular usecase just specifying a custom error type would be enough. RpcError trait may be useful as well if the openraft codebase is going to actually use it.

From my experience (I've been poking the lib for about 10 days) nested error types in Raft (the struct) API isn't such a big pain point, however I can't say the same thing about RaftNetwork.

[xDarksome]

It's possible to write an abstraction (what I'm currently doing) on top of Raft and type erase errors. However, errors used inside RaftNetwork leak through because openraft specifically depend on them and I'm forced to return these errors as part of the public (network) API of a node.

[drmingdrmer]

Thank you two guys for your well-thought-out suggestion on enhancing network error management.

We can refactor network error handling in the following manner:

RaftNetwork methods should only return one of the two network error types:

• Temporary network failure: openraft should immediately retry the RPC.
• Permanent network failure: openraft should back off for a while before retrying.

Regarding other errors currently in use:

• The Timeout error appears to be unnecessary and should be classified as one of the two network error types mentioned above.

• The Fatal error, which includes three variants (StorageError, Panicked, and Stopped), should be treated as a Permanent failure and managed in the same manner as other Permanent failures.

• The sole remote error, InstallSnapshotError, should be moved into the InstallSnapshotResponse instead of being a separate error, similar to how AppendEntriesResponse and VoteResponse are handled.

Building upon your proposal for the new RaftNetwork API, it can be defined as follows:

trait RaftNetworkError: std::error::Error {
    fn is_permanent_error() -> bool;
}

pub trait RaftNetwork<C>: Send + Sync + 'static
where C: RaftTypeConfig
{
    type Err: RaftNetworkError; // maybe some trait bounds here

    // The following are same as your proposed:

    async fn send_append_entries(
        &mut self,
        rpc: AppendEntriesRequest<C>,
    ) -> Result<AppendEntriesResponse<C::NodeId>,  Self::Err>;

    async fn send_install_snapshot(
        &mut self,
        rpc: InstallSnapshotRequest<C>,
    ) -> Result<
        InstallSnapshotResponse<C::NodeId>,
        RPCError<C::NodeId, C::Node, Self::Err>,
    >;

    async fn send_vote(
        &mut self,
        rpc: VoteRequest<C::NodeId>,
    ) -> Result<VoteResponse<C::NodeId>, Self::Err>;
}

[schreter]

Sorry for the late reply, but yes, this looks good.

Just one point, maybe instead of is_permanent_error() return some ErrorKind, which currently has only two enum values (Transient, Permanent). Then, if we find out that there indeed is a need, for example, for a Timeout, you can easily add it.