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

*: port coreos/etcd#9985 #340

Merged
merged 3 commits into from
Mar 3, 2020
Merged

*: port coreos/etcd#9985 #340

merged 3 commits into from
Mar 3, 2020

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Feb 27, 2020

The patch is to speed up log replication when a node is way behind than leader and logs are not compacted yet.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@@ -1269,7 +1301,7 @@ impl<T: Storage> Raft<T> {
to_send.set_msg_type(MessageType::MsgReadIndexResp);
to_send.set_index(rs.index);
to_send.set_entries(req.take_entries());
*more_to_send = Some(to_send);
ctx.more_to_send.push(to_send);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about more_to_send: HashMap<u64, Message>? So we can send only 1 read index response to one peer.

Copy link
Member Author

Choose a reason for hiding this comment

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

HashMap is slower than Vec, it may not be a good idea. On the other hand, the optimization assumes there is a queue in follower side, which may not be a valid case.

@hicqu
Copy link
Contributor

hicqu commented Mar 2, 2020

rest LGTM. BTW could you send a PR to master? Let's think about how to fix test cases about initializing with configuration.

@BusyJay
Copy link
Member Author

BusyJay commented Mar 2, 2020

I will port to master when tests issues are fixed.

hicqu
hicqu previously approved these changes Mar 2, 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

src/raft.rs Outdated
}
}

if send_append {
if ctx.has_reply {
Copy link
Member

Choose a reason for hiding this comment

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

The has_reply is a bit redundant. Maybe we can change it to

Suggested change
if ctx.has_reply {
if ctx.send_append || ctx.loop_append {

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
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.

LGTM

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