Update comments #357
Update comments #357
Conversation
src/raw_node.rs
Outdated
| /// HasReady called when RawNode user need to check if any Ready pending. | ||
| /// Checking logic in this method should be consistent with Ready.containsUpdates(). | ||
| /// Checks if any Ready is pending. | ||
| /// This method should be consistent with Ready.containsUpdates(). |
There was a problem hiding this comment.
Just for a hint, containsUpdates() seems not being implemented in this codebase.
It comes from https://github.com/etcd-io/etcd/blob/master/raft/node.go#L106
There was a problem hiding this comment.
Hmm maybe remove this line?
src/raft.rs
Outdated
| @@ -361,6 +361,7 @@ impl<T: Storage> Raft<T> { | |||
|
|
|||
| /// For testing leader lease | |||
| #[doc(hidden)] | |||
| #[cfg(test)] | |||
There was a problem hiding this comment.
This is not going to work. When running integration tests, test profile is disabled for the crate.
| // Before pre_vote enable, there may be a recieving candidate with higher term, | ||
| // but less log. After update to pre_vote, the cluster may deadlock if | ||
| // we drop messages with a lower term. | ||
| // Unlike normal election, both sender and receiver of MsgReqPreVote{Resp} |
There was a problem hiding this comment.
I think the original comment is quite sufficient. The comment in this PR may give a wrong impression that the check is for a rarely happen situation.
There was a problem hiding this comment.
Maybe just for me but it took me a while to figure out what the deadlock is. :)
Surely the example is misleading a verbose. How about I remote that part?
src/raft.rs
Outdated
| // the message (it ignores all out of date messages). | ||
| // The term in the original message and current local term are the | ||
| // same in the case of regular votes, but different for pre-votes. | ||
| // When responding to MsgReq{Pre}Vote, we include the term from the |
There was a problem hiding this comment.
May I know why? Is these comment semantically parsed? I thought only the comments start with /// are used in docs..
I updated some comments (and trivial code) while reading the library (it was a fun journey.) I'm new to both rust and raft. Not sure whether this can be useful for others. Let me know if I made anything wrong. :) Signed-off-by: Haibo Huang <[email protected]>
|
Thanks for taking time to read this boring change. 😀 By the way I have two questions about the design:
Some reasons I can think of: Is this correct? Thanks a lot for your time. |
I updated some comments (and a little bit code) while reading the library (it was a fun journey.)
I'm new to both rust and raft. Not sure whether this can be useful for others. Let me know if I made anything wrong. :)