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.

Sign up for GitHub

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

Jump to bottom

Refactor: Move client_resp_channels from LeaderData to new tx_...-field in RaftCore #963

Closed
tvsfx opened this issue Dec 7, 2023 · 2 comments · Fixed by #973
Closed

Refactor: Move client_resp_channels from LeaderData to new tx_...-field in RaftCore #963

tvsfx opened this issue Dec 7, 2023 · 2 comments · Fixed by #973
Assignees
@drmingdrmer

Comments

@tvsfx
Copy link
Contributor

tvsfx commented Dec 7, 2023 •
edited
Loading

Currently, the QuitLeader command makes the RaftCore drop the node's client response channels.
However, even when the node becomes a follower, it can still keep its existing response channels around, and reply to them when it applies the corresponding entries, even though it is no longer a leader at that time. This is correct, since no uncommitted entries will ever be applied. This simplifies the QuitLeader logic.
If client_resp_channels is moved out of the LeaderData, this becomes possible.
The check to see whether we are a leader at entry apply-time would then also disappear from here.

As an aside, having the list of client response channels in each node would also be convenient for follower reads or read indexes in the future.
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 7, 2023

👋 Thanks for opening this issue!

Get help or engage by:

  • /help : to print help messages.
  • /assignme : to assign this issue to you.

@tvsfx tvsfx changed the title Move client_resp_channels from LeaderData to new tx_...-field in RaftCore Move client_resp_channels from LeaderData to new tx_...-field in RaftCore Dec 7, 2023
@tvsfx tvsfx changed the title Move client_resp_channels from LeaderData to new tx_...-field in RaftCore Refactor: Move client_resp_channels from LeaderData to new tx_...-field in RaftCore Dec 7, 2023
@drmingdrmer
Copy link
Member

Nice catch! My hero!

@drmingdrmer drmingdrmer self-assigned this Dec 8, 2023
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Dec 19, 2023
@drmingdrmer
Refactor: respond to client even when leader is switched
ea01728 
Before this commit, the channels used for sending responses to the
client were dropped whenever a leader stepped down, which was not
necessary.

In the current commit, when a leader quits, we now preserve the channel, ensuring that:

- An OK response is sent to the client if the log entry the client has
  written is successfully committed;

- Alternatively, a `ForwardToLeader` error is sent if the log is
  reverted due to a conflict with the new leader.

In short, OpenRaft now makes a concerted effort to deliver a success
message to the client wherever possible.

Two test cases have been added to cover both successful and erroneous
scenarios.

Special thanks to @tvsfx for proposing this refinement :D

- Fix: databendlabs#963
@drmingdrmer drmingdrmer mentioned this issue Dec 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@drmingdrmer drmingdrmer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor: respond to client even when leader is switched drmingdrmer/openraft
2 participants
@drmingdrmer @tvsfx