-
Notifications
You must be signed in to change notification settings - Fork 157
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
make last_log_id an Option<LogId> #68
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid unwrap()
is not a safe way working with Option
.
@@ -31,8 +31,8 @@ impl<'a, D: AppData, R: AppDataResponse, N: RaftNetwork<D>, S: RaftStorage<D, R> | |||
&mut self, | |||
mut members: BTreeSet<NodeId>, | |||
) -> Result<(), InitializeError> { | |||
if self.core.last_log_id.index != 0 || self.core.current_term != 0 { | |||
tracing::error!({self.core.last_log_id.index, self.core.current_term}, "rejecting init_with_config request as last_log_index or current_term is 0"); | |||
if self.core.last_log_id.unwrap().index != 0 || self.core.current_term != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this PR, the zero value of last_log_id
is (0,0)
.
Since now on, the zero value of it should be None.
Thus here to check if the state is uninitialized by checking if last_log_id
is None.
unwrap()
is not safe and should be avoided.
if self.core.last_log_id.index != 0 || self.core.current_term != 0 { | ||
tracing::error!({self.core.last_log_id.index, self.core.current_term}, "rejecting init_with_config request as last_log_index or current_term is 0"); | ||
if self.core.last_log_id.unwrap().index != 0 || self.core.current_term != 0 { | ||
tracing::error!({"{} {}", self.core.last_log_id.unwrap(), self.core.current_term}, "rejecting init_with_config request as last_log_index or current_term is 0"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the format literal string? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tracing
seems not allow to do expression like unwrap()
or unwrap_or_else()
, it needs manually format, so I did it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler is not smart enough 🤔
These syntaxes just compile smoothly:
let x = Some(1);
tracing::debug!(x=%x.unwrap(), "message: x: Display");
tracing::debug!(x=?x.unwrap(), "message: x: Debug");
tracing::debug!({ x=%x.unwrap() }, "message");
@@ -181,7 +184,7 @@ impl<'a, D: AppData, R: AppDataResponse, N: RaftNetwork<D>, S: RaftStorage<D, R> | |||
ChangeMembershipError::LearnerIsLagging { | |||
node_id: *new_node, | |||
matched: node.matched, | |||
distance: self.core.last_log_id.index.saturating_sub(node.matched.index), | |||
distance: self.core.last_log_id.unwrap().index.saturating_sub(node.matched.index), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here using unwrap_or_else()
would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my humble opinion, here using unwrap()
is equal to assert!
, it is the right way to do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it means that we can only do this action when it is initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, it is legal to change membership with an empty, uninitialized state.
Although in this codebase it hardly will.
@@ -217,7 +220,7 @@ impl<'a, D: AppData, R: AppDataResponse, N: RaftNetwork<D>, S: RaftStorage<D, R> | |||
|
|||
// Caveat: membership must be updated before commit check is done with the new config. | |||
self.core.effective_membership = EffectiveMembership { | |||
log_id: self.core.last_log_id, | |||
log_id: self.core.last_log_id.expect("raft core last_log_id is uninitialized"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it's better to use the log_id
of the returned entry in res
.
There is another issue that updating the effective_membership
at line 222 should be placed after checking res
, e.g. at line 242
@@ -26,7 +26,7 @@ impl<D: AppData, R: AppDataResponse, N: RaftNetwork<D>, S: RaftStorage<D, R>> Ra | |||
&mut self, | |||
msg: AppendEntriesRequest<D>, | |||
) -> RaftResult<AppendEntriesResponse> { | |||
tracing::debug!(%self.last_log_id, %self.last_applied, msg=%msg.summary(), "handle_append_entries_request"); | |||
tracing::debug!({"{} {} {}", self.last_log_id.unwrap(), self.last_applied, msg.summary()}, "handle_append_entries_request"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
this pr fix #49 .
make struct
RaftCore
'slast_log_id
intoOption<LogId>
to express an uninitialized state.