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

implement committed entries pagination #356

Closed

Conversation

Fullstop000
Copy link
Member

Finish the remaining work in #161

Signed-off-by: Fullstop000 [email protected]

@Fullstop000
Copy link
Member Author

@BusyJay @hicqu PTAL. Sorry for delaying this for sooooooo long :(.

src/config.rs Outdated Show resolved Hide resolved
src/raw_node.rs Outdated Show resolved Hide resolved
@Fullstop000 Fullstop000 force-pushed the committed_entries_pagination branch 2 times, most recently from bfc4ea9 to 60d7771 Compare June 11, 2020 15:05
@Fullstop000
Copy link
Member Author

@BusyJay @hicqu PTAL

src/config.rs Outdated Show resolved Hide resolved
src/raft.rs Outdated Show resolved Hide resolved
src/raw_node.rs Outdated Show resolved Hide resolved
let mut ents = Vec::with_capacity(ents_count);
let mut size = 0u64;
for i in 0..ents_count {
let e = new_entry(1, i as u64 + 1, Some("a"));
Copy link
Member

Choose a reason for hiding this comment

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

Why not i +1 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

ents_count is a usize, though I could use 0..ents_count as u64

.unwrap()
.get_index();
raw_node.advance(rd);
let mut m = new_message(1, 1, MessageType::MsgHeartbeat, 0);
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of the heartbeat?

Copy link
Member Author

@Fullstop000 Fullstop000 Jun 18, 2020

Choose a reason for hiding this comment

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

Let the node learn the current commit_index is 11. Altered to use commit_to instead.

// - the next Ready asks the app to apply index 11 (omitting index 10), losing a
// write.
#[test]
fn test_commit_pagination_after_restart() {
Copy link
Member

Choose a reason for hiding this comment

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

The test case doesn't seem to be the same as upstream.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is silently a little different from the one in etcd due to the diff of MemStorage

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.

Rest LGTM.

// Set a max_size_per_msg that would suggest to Raft that the last committed entry should
// not be included in the initial rd.committed_entries. However, our storage will ignore
// this and *will* return it (which is how the Commit index ended up being 10 initially).
cfg.max_size_per_msg = size - 1;
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 it should be size - uint64(s.ents[len(s.ents)-1].Size()) - 1. The purpose is to let raft return 9 entries instead of 10, so that entry at 10 will get missed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, the etcd codes of this test case are kind of buggy because the firstIndex here is 2 because the codes in this test replace the ents with start index 1 and the firstIndex of MemoryStorage is always s.ents[0].Index + 1 due to the dummy entry (the index 0). Therefore, len(rd.CommittedEntries) in the first loop is 8 in etcd version.

And since MemStorage in raft-rs has no such a dummy entry approach, we can correctly use size - 1 here to return 9 committed entries in the first loop.

Though I found I should use cfg.max_committed_size_per_ready here 🤣

@BusyJay
Copy link
Member

BusyJay commented Jun 7, 2021

@hicqu has re-implemented it as #440 with slight adjust. Close it now. Thanks for your contributions!

@BusyJay BusyJay closed this Jun 7, 2021
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.

2 participants