-
Notifications
You must be signed in to change notification settings - Fork 155
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: Transfer Leader #1221
Feature: Transfer Leader #1221
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Reviewed 27 of 27 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @drmingdrmer)
openraft/src/engine/handler/leader_handler/mod.rs
line 112 at r1 (raw file):
/// Disable proposing new logs for this Leader, and transfer Leader to another node // TODO: test
?
openraft/src/raft/mod.rs
line 650 at r1 (raw file):
/// /// [`RaftNetworkV2::transfer_leader`]: crate::network::v2::RaftNetworkV2::transfer_leader #[since(version = "0.10.0")]
?
In general, it is a bit convoluted. Wouldn't it make more sense to create a tiny wrapper here for the trigger named transfer_leader()
and rename this method to handle_transfer_leader_request()
or so, so it's clear what is what? Or at least rename this method to indicate it's not the request, but the handler?
Suggestion:
/// Otherwise, it just resets the Leader lease to allow the `to` node to become the Leader.
///
/// The application calls
/// [`Raft::trigger().transfer_leader()`](crate::raft::trigger::Trigger::transfer_leader) to
/// submit Transfer Leader command. Then, the current Leader will broadcast it to every node in
/// the cluster via [`RaftNetworkV2::transfer_leader`] and the implementation on the remote node
/// responds to transfer leader request by calling this method.
///
/// [`RaftNetworkV2::transfer_leader`]: crate::network::v2::RaftNetworkV2::transfer_leader
#[since(version = "0.10.0")]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @schreter)
openraft/src/engine/handler/leader_handler/mod.rs
line 112 at r1 (raw file):
Previously, schreter wrote…
?
˙–˙
openraft/src/raft/mod.rs
line 650 at r1 (raw file):
Previously, schreter wrote…
?
In general, it is a bit convoluted. Wouldn't it make more sense to create a tiny wrapper here for the trigger named
transfer_leader()
and rename this method tohandle_transfer_leader_request()
or so, so it's clear what is what? Or at least rename this method to indicate it's not the request, but the handler?
handle_transfer_leader_request()
would be good.
But I did not get what you mean about "a tiny wrapper here for the trigger". 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @drmingdrmer)
openraft/src/raft/mod.rs
line 650 at r1 (raw file):
"a tiny wrapper here for the trigger". 🤔
Simply something like this:
async fn transfer_leader(to: ID) -> Result<(), ...> {
self.trigger().transfer_leader(to).await
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @schreter)
openraft/src/raft/mod.rs
line 650 at r1 (raw file):
Previously, schreter wrote…
"a tiny wrapper here for the trigger". 🤔
Simply something like this:
async fn transfer_leader(to: ID) -> Result<(), ...> { self.trigger().transfer_leader(to).await }
Oh. I'd like to put all user triggered command to trigger()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @drmingdrmer)
openraft/src/raft/mod.rs
line 650 at r1 (raw file):
Oh. I'd like to put all user triggered command to
trigger()
.
No problem, it was just an idea.
c1a57f7
to
08f7fd4
Compare
Call `Raft.trigger().transfer_leader(to)` to inform the raft node to transfer its leadership to Node `to`. This feature is enabled only when: the application uses `RaftNetworkV2` and implements the `RaftNetworkV2::transfer_leader()` methods. This method provides a default implementation that returns an `Unreachable`, and such an error will be just ignored. Application upgrading Openraft from older version does not need to modify any codes, unless TransferLeader is required. Upgrade tip: Implement `RaftNetworkV2::transfer_leader()` to send the `TransferLeaderRequest` to the target node. The target node that receives this request should then pass it to `Raft::transfer_leader()`.
08f7fd4
to
7e3ee4f
Compare
Changelog
Feature: Transfer Leader
Call
Raft.trigger().transfer_leader(to)
to inform the raft node totransfer its leadership to Node
to
.This feature is enabled only when: the application uses
RaftNetworkV2
and implements the
RaftNetworkV2::transfer_leader()
methods. This methodprovides a default implementation that returns an
Unreachable
, andsuch an error will be just ignored.
Application upgrading Openraft from older version does not need to modify any
codes, unless TransferLeader is required.
Upgrade tip:
Implement
RaftNetworkV2::transfer_leader()
to send theTransferLeaderRequest
to the target node.The target node that receives this request should then pass it to
Raft::transfer_leader()
.This change is