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. #149

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

joshuazh-x
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: #148

Copy link
Contributor

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

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

LGTM % a couple minor things

raft_test.go Outdated Show resolved Hide resolved
tracker/progress.go Show resolved Hide resolved
@pav-kv
Copy link
Contributor

pav-kv commented Feb 4, 2024

@joshuazh-x Thanks for the fix.

@ahrtr Could you review/merge? Thank you.

@pav-kv pav-kv requested a review from ahrtr February 4, 2024 03:32
@ahrtr
Copy link
Member

ahrtr commented Feb 4, 2024

LGTM with one nit.

Confirmed that the test case will fail without the fix.

--- FAIL: TestLogReplicationWithReorderedMessage (0.00s)
    raft_test.go:4869: 
        	Error Trace:	/Users/wachao/go/src/github.com/joshuazh-x/raft/raft_test.go:4869
        	Error:      	Not equal: 
        	            	expected: 0x2
        	            	actual  : 0x0
        	Test:       	TestLogReplicationWithReorderedMessage
FAIL
exit status 1
FAIL	go.etcd.io/raft/v3	0.565s

Probably we should add an invariant check each time when sending a MsgApp message to ensure that the index should always be greater than the Match. It can be addressed in a separate PR.

raft_test.go Outdated Show resolved Hide resolved
@ahrtr
Copy link
Member

ahrtr commented Feb 6, 2024

Please rebase this PR although github doesn't show any conflict.

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.

Signed-off-by: Joshua Zhang <[email protected]>
@joshuazh-x
Copy link
Contributor Author

Please rebase this PR although github doesn't show any conflict.

done.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

lgtm

Thanks

@ahrtr ahrtr merged commit 073f90d into etcd-io:main Feb 6, 2024
10 checks passed
@ahrtr ahrtr added this to the v3.6.0 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants