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: next index shall be larger than match index #557

Merged
merged 6 commits into from
Dec 6, 2024

Conversation

wego1236
Copy link
Contributor

Leader shall always replicate its log from a index larger than corresponding match index. Note that this is not a correctness issue, but rather an optimization in case of message reordering.

Related: #555

@BusyJay
Copy link
Member

BusyJay commented Nov 14, 2024

Thanks, seems the test case is missing.

@wego1236
Copy link
Contributor Author

Do I need to add test here?

@BusyJay
Copy link
Member

BusyJay commented Nov 15, 2024

You also need to port the test case from the original PR.

@wego1236
Copy link
Contributor Author

This test case needs to be added to harness/tests/integreation_cases/test_raft.rs right?

@BusyJay
Copy link
Member

BusyJay commented Nov 15, 2024

Yes.

…alue to be less than or equal to the match value in the probe state

Signed-off-by: wego1236 <[email protected]>
Signed-off-by: wego1236 <[email protected]>
@wego1236
Copy link
Contributor Author

I may need some help, the test case I wrote modelled after etcd doesn't achieve the same results as they do, this test case may need your help to complete it. When I tried it, I found that in some cases etcd and raft-rs produced different results, so I may not be able to simply copy the corresponding test cases.

Signed-off-by: wego1236 <[email protected]>
@wego1236
Copy link
Contributor Author

I'm not familiar with rust, maybe for me this test case needs to know a bit too much, if it's easier for you, could you help me to modify it a bit

assert_eq!(m.reject, false);
assert_eq!(m.index, 2);
let _ = r1.step(m);
let _ = expect_one_message(&mut r1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let _ = expect_one_message(&mut r1);

You may need to remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot, i'll fix it now

…d automatically due to some plugins and are now reverted to their original versions

Signed-off-by: wego1236 <[email protected]>
@wego1236
Copy link
Contributor Author

Is there anything else I need to do?

@wego1236 wego1236 requested a review from chagelo November 25, 2024 11:37
@chagelo
Copy link

chagelo commented Nov 26, 2024

@BusyJay PTAL

Copy link
Member

@BusyJay BusyJay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BusyJay BusyJay merged commit 2fbeee5 into tikv:master Dec 6, 2024
6 checks passed
@BusyJay
Copy link
Member

BusyJay commented Dec 6, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants