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

raft: fix Ready.MustSync logic #128

Closed
siddontang opened this issue Sep 26, 2018 · 3 comments · Fixed by #372
Closed

raft: fix Ready.MustSync logic #128

siddontang opened this issue Sep 26, 2018 · 3 comments · Fixed by #372
Labels
Bug Recognized misbehavior. Help Wanted An issue with unsolved problems, looking for help.
Milestone

Comments

@siddontang
Copy link
Contributor

Refer etcd-io/etcd#10106

Seem that we have already used the Raft hard state, not the ready's, but we need to port the test.

Btw, seem that we don't consider Entries number for MustSync, see https://github.com/etcd-io/etcd/blob/b9b75f81e56241b0f573ba31b3c3a018290e9343/raft/node.go#L584

Do you know why? @BusyJay

@BusyJay
Copy link
Member

BusyJay commented Sep 26, 2018

I have no idea. 😿

@Hoverbear Hoverbear added the Bug Recognized misbehavior. label Sep 26, 2018
@Hoverbear Hoverbear added this to the 0.5.0 milestone Sep 26, 2018
@Hoverbear Hoverbear modified the milestones: 0.5.0, 0.6.0 Feb 11, 2019
@hicqu hicqu added the Help Wanted An issue with unsolved problems, looking for help. label Nov 7, 2019
@Hoverbear Hoverbear removed their assignment Mar 2, 2020
@jayzhan211
Copy link
Contributor

can someone specify which test cases need to be ported from etcd? or just modify the mustsync logic?

@BusyJay
Copy link
Member

BusyJay commented Jun 10, 2020

I think all the tests are ported. The issue is related to node, which is not available from raft-rs. We just implement raw_node.

@siddontang I believe it's because we don't sync all entries. We just sync those entries that are important like conf change or split. Although, it may not be good as a standalone library. Giving that we have been optimizing the performance of file syncing, I think it's OK to just sync all entries.

@accelsao I think you can modify mustsync logic and add a unit test.

BusyJay pushed a commit that referenced this issue Jun 16, 2020
close #128

Signed-off-by: accelsao <bebe8277@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Recognized misbehavior. Help Wanted An issue with unsolved problems, looking for help.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants