batch MsgAppend entries #179
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks @Sch00lb0y I am looking forward to your benchmark results :-) |
|
@siddontang |
|
Awesome improvements @Sch00lb0y ! |
siddontang
reviewed
Feb 6, 2019
siddontang
reviewed
Feb 11, 2019
Hoverbear
reviewed
Feb 11, 2019
Hoverbear
reviewed
Feb 12, 2019
|
The rest LGTM! :) Thanks so much for this contribution. |
|
PTAL @BusyJay |
|
I don't think batch policy implemented here is involved in the posted benchmark result. Can you add a flag to make this feature be able to be opted out when necessary? |
|
Hi @BusyJay , Thanks for pointing it out. Even, I felt the same. cuz it showing different number for every run. That's why I ran cargo clean and made those results.
flag as in option like |
|
Yes. So that if the optimization doesn't perform as expected, user can still have a choice. |
|
I have addressed your suggestions. |
Hoverbear
previously approved these changes
Feb 13, 2019
|
Please fix the conflict. |
BusyJay
reviewed
Feb 15, 2019
| @@ -4110,3 +4111,22 @@ fn test_new_raft_with_bad_config_errors() { | |||
| let raft = Raft::new(&invalid_config, new_storage()); | |||
| assert!(raft.is_err()) | |||
| } | |||
| // tests whether MsgAppend are batched | |||
src/raft.rs
Outdated
| batched_entries.append(ents); | ||
| msg.set_entries(RepeatedField::from_vec(batched_entries)); | ||
| is_empty = msg.get_entries().is_empty(); | ||
| if !is_empty { |
There was a problem hiding this comment.
It should break when target message is found.
- add test for msg append batching Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
|
@BusyJay conflict resolved and added those changes |
|
Labelling this DNM since it's close to merge and we're about to release #182. This will be part of 0.6.0 |
|
LGTM |
brson
previously approved these changes
Feb 21, 2019
BusyJay
reviewed
Feb 25, 2019
src/raft.rs
Outdated
| let mut last_idx = 0; | ||
| let mut is_empty = true; | ||
| for msg in &mut self.msgs { | ||
| if msg.get_log_term() == term |
There was a problem hiding this comment.
Different log term can be sent in the same message.
src/raft.rs
Outdated
| // will append the entries to the existing MsgAppend | ||
| let mut is_batched = false; | ||
| let mut last_idx = 0; | ||
| let mut is_empty = true; |
There was a problem hiding this comment.
last_idx != 0 indicates it's not empty.
src/raft.rs
Outdated
| let mut batched_entries = msg.take_entries().into_vec(); | ||
| batched_entries.append(ents); | ||
| msg.set_entries(RepeatedField::from_vec(batched_entries)); | ||
| is_empty = msg.get_entries().is_empty(); |
There was a problem hiding this comment.
Should check if ents is empty instead.
src/raft.rs
Outdated
| msg.set_index(pr.next_idx - 1); | ||
| msg.set_commit(self.raft_log.committed); | ||
| let mut batched_entries = msg.take_entries().into_vec(); | ||
| batched_entries.append(ents); |
There was a problem hiding this comment.
What if logs in origin msg and in ents are not continuous?
There was a problem hiding this comment.
will that case come? (sorry if I've asked stupid question :( )
There was a problem hiding this comment.
Yes, follower can reject the leader's MsgAppend to make it reset next_index.
There was a problem hiding this comment.
If it that case should I discard the previous ents?
There was a problem hiding this comment.
Yes. For simplicity, you can also just give up batching in that case.
- add test to assert non continuous entries Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
|
@BusyJay I've made those changes and added a necessary assertion for non-continuous entries in the test case. Please have a look Thanks :) |
BusyJay
reviewed
Feb 26, 2019
src/raft.rs
Outdated
| "{} is sending append in unhandled state {:?}", | ||
| self.tag, pr.state | ||
| ), | ||
| msg.set_index(pr.next_idx - 1); |
There was a problem hiding this comment.
You should not update index anymore. Seems missing a case to cover it.
src/util.rs
Outdated
| pub fn is_continuous_ents(msg: &Message, ents: &[Entry]) -> bool { | ||
| if !msg.get_entries().is_empty() && !ents.is_empty() { | ||
| let expected_next_idx = msg.get_entries().last().unwrap().get_index() + 1; | ||
| if expected_next_idx != ents.first().unwrap().get_index() { |
There was a problem hiding this comment.
You mean return expected_next_idx == ents.first().unwrap().get_index()?
- add assertion to check index - refactor is_continuous util Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
Signed-off-by: பாலாஜி ஜின்னா <[email protected]>
|
@BusyJay done :) |
BusyJay
approved these changes
Feb 27, 2019
Hoverbear
approved these changes
Feb 27, 2019
|
Thanks so much @Sch00lb0y :) Really appreciate your continued improvements! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#18
entries will be appended if there is an existing receiver MsgAppend message.
let me know if anything needs to be added.
Signed-off-by: balaji [email protected]