raft: don't panic when looking for term conflicts #36
Conversation
pav-kv
force-pushed
the
protect-from-panic
branch
2 times, most recently
from
b3d210c to
d0a5d0d
Compare
|
@tbg PTAL |
There was a problem hiding this comment.
Looks good to me, thanks! I left some suggestions to a have fewer "unexpected" return values and no case in which the term is not populated. The goal would be to remove all edge cases and make it more transparent what happens in the truncation scenarios, etc.
It might also be good to add some targeted unit testing for findConflictByTerm. I saw that you added a test case in TestFastLogRejection, but a few more wouldn't hurt. I should have added that right when we introduced findConflictByTerm, sorry about that.
Approving preemptively because the current code is "good enough" and I'm fairly certain it's correct (mod comments). The improvements would make it easier to be convinced that it's correct in a month or two but I'll rely on your judgment of which further improvements are worth it.
pav-kv
force-pushed
the
protect-from-panic
branch
2 times, most recently
from
685e800 to
bdae85a
Compare
Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
This commit fixes a hypothetical panic that may occur when a stale MsgApp message arrives to a follower. The conflict searching algorithm in findConflictByTerm may return a log index which is not present in the log, and thus the raftLog.term() method may return an error. It is safe to ignore this error and send MsgAppResp with the found index and a zero LogTerm. This commit also restores the behaviour that existed before commit d0fb0cd. Back then, the term() function would silently return 0 instead of an error, and a zero LogTerm would be sent with the rejecting MsgAppResp. After that commit, there is a new possible panic. We remove the possibility of this panic here. Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
pav-kv
force-pushed
the
protect-from-panic
branch
from
bdae85a to
0c22de0
Compare
|
@tbg Addressed comments, and added unit tests for |
This commit fixes a hypothetical panic that may occur when a stale
MsgAppmessage arrives to a follower. The conflict searching algorithm in
findConflictByTermmay return a log index which is not present in the log, andthus the
raftLog.term()method may return an error. It is safe to ignore thiserror and send
MsgAppRespwith the found index and a zeroLogTerm.This commit also restores the behaviour that existed before commit d0fb0cd.
Back then, the
term()function would silently return 0 instead of an error, anda zero
LogTermwould be sent with the rejectingMsgAppResp. After that commit,there is a new possible panic. We remove the possibility of this panic here.