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

add committed index in Progress #366

Merged
merged 13 commits into from
Jun 9, 2020
Merged

add committed index in Progress #366

merged 13 commits into from
Jun 9, 2020

Conversation

jayzhan211
Copy link
Contributor

@jayzhan211 jayzhan211 commented May 22, 2020

Close #325

  • Commit index won't step backward.
  • Log rejection should not change commit index
  • Successfully Append should update commit index
  • Heartbeat should update commit index

Signed-off-by: accelsao [email protected]

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

I think my test_case is useless..

@jayzhan211 jayzhan211 marked this pull request as draft May 22, 2020 12:51
Signed-off-by: accelsao <[email protected]>
@jayzhan211 jayzhan211 marked this pull request as ready for review May 22, 2020 15:09
@jayzhan211
Copy link
Contributor Author

error The command "if [[ $TRAVIS_RUST_VERSION == "stable" && $TRAVIS_OS_NAME == "linux" ]]; then cargo fmt --all -- --check; fi" exited with 1. in travis build is left

Signed-off-by: accelsao <[email protected]>
@@ -119,6 +123,7 @@ impl Progress {
self.recent_active = false;
debug_assert!(self.ins.cap() != 0);
self.ins.reset();
self.committed_index = 0
Copy link
Member

Choose a reason for hiding this comment

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

Seems unnecessary to reset.

@@ -212,6 +212,121 @@ fn test_progress_update() {
}
}

#[test]
fn test_progress_committed() {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use Network to test the commit index change. It should validate following case:

  1. Log rejection should not change commit index
  2. Successfully Append should update commit index
  3. Heartbeat should update commit index
  4. Commit index won't step backward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still do not know when will we get the smaller commit index

Copy link
Member

Choose a reason for hiding this comment

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

For example, leader sends two MsgAppend to follower and gets MsgAppendResponse in reverse order.

@jayzhan211 jayzhan211 marked this pull request as draft May 27, 2020 01:19
@jayzhan211 jayzhan211 marked this pull request as ready for review May 27, 2020 23:25
Signed-off-by: accelsao <[email protected]>
Signed-off-by: accelsao <[email protected]>
Signed-off-by: accelsao <[email protected]>

nt.send(vec![msg_append_response.clone()]);

assert_eq!(nt.peers[&1].prs().get(2).unwrap().committed_index, 10);
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldnt that commiitted index is update as 10?

Copy link
Member

Choose a reason for hiding this comment

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

If leader doesn't have log 10, it shouldn't commit to 10. Giving that raft-rs doesn't take much effort in byzantine fault tolerant, I think remove the case is OK for now.

Signed-off-by: accelsao <[email protected]>
BusyJay
BusyJay previously approved these changes Jun 2, 2020
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.

Thanks for your contributions!

hicqu
hicqu previously approved these changes Jun 8, 2020
Copy link
Member

@gengliqi gengliqi left a comment

Choose a reason for hiding this comment

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

Rest LGTM

Signed-off-by: accelsao <[email protected]>
@jayzhan211 jayzhan211 dismissed stale reviews from hicqu and BusyJay via 500e011 June 8, 2020 12:30
Copy link
Member

@gengliqi gengliqi left a comment

Choose a reason for hiding this comment

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

One more little comment

Signed-off-by: accelsao <[email protected]>
@jayzhan211 jayzhan211 requested review from gengliqi, hicqu and BusyJay June 9, 2020 05:02
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.

Add a way to figure out followers commit index
5 participants