Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: add Raft::client_current_leader API #97

Closed
xu-cheng opened this issue Jan 1, 2021 · 2 comments · Fixed by #99
Closed

Feature Request: add Raft::client_current_leader API #97

xu-cheng opened this issue Jan 1, 2021 · 2 comments · Fixed by #99
Labels
enhancement New feature or request good first issue Good for newcomers optimization An optimization to the system

Comments

@xu-cheng
Copy link
Contributor

xu-cheng commented Jan 1, 2021

In addition to the existing Raft::client_read API, may I suggest to add an Raft::client_current_leader API like the following:

pub async fn client_current_leader(&self) -> Result<NodeId, ClientCurrentLeaderError> {
    match self.client_read().await {
        Ok(()) => Ok(cur_node_id),
        Err(ClientReadError::ForwardToLeader(Some(id))) => Ok(id),
        Err(ClientReadError::ForwardToLeader(None)) => Err(ClientCurrentLeaderError::LeaderUnknown),
        Err(ClientReadError::RaftError(e)) => Err(ClientCurrentLeaderError::RaftError(e)),
    }
}

This would be helpful to certain applications, where the leader is required to do certain preprocessing before submitting the real Raft write request.

Also, it may be a good idea to have another similar API, which returns the current leader without sending any Raft requests for performance reason. Something like:

pub async fn client_current_presumptive_leader(&self) -> Result<NodeId, ClientCurrentLeaderError> {
    if self.is_leader() {
        Ok(cur_node_id)
    } else if let Some(leader_id) = self.leader_id {
        Ok(leader_id)
    } else {
        Err(ClientCurrentLeaderError::LeaderUnknown)
    }
}

When using this API, it is up to the application's responsibility to check who is the real leader later.

@thedodd
Copy link
Collaborator

thedodd commented Jan 4, 2021

I do think that this is a great idea. For the latter, I will often just use the Raft's metrics handle, which has a field indicating if the node is currently the leader or not. We could probably build upon that mechanism for the second item you've mentioned.

@thedodd thedodd added enhancement New feature or request good first issue Good for newcomers optimization An optimization to the system labels Jan 4, 2021
@xu-cheng
Copy link
Contributor Author

xu-cheng commented Jan 5, 2021

Thanks for the advice. I created PR #99.

I only implemented for the second API mentioned above. Because, I found that the first API may not be that useful. At the same time, I am not sure the first API can actually return the confirmed leader if it is called from a non-leader node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers optimization An optimization to the system
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants