-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
raftstore: clean callback when peer is destroyed #1683
Conversation
CI failed |
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.
LGTM
tests/raftstore/test_lease_read.rs
Outdated
@@ -380,3 +385,37 @@ fn test_server_lease_unsafe_during_leader_transfers() { | |||
let mut cluster = new_node_cluster(0, count); | |||
test_lease_unsafe_during_leader_transfers(&mut cluster); | |||
} | |||
|
|||
#[test] |
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.
Add a brief description of what this test case is for.
Rest LGTM |
for read in self.reads.drain(self.ready_cnt..) { | ||
for (req, cb) in read.cmds { | ||
for mut read in self.reads.drain(self.ready_cnt..) { | ||
for (req, cb) in read.cmds.drain(..) { |
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.
Is it just a refactoring behavior and have nothing to do with this PR?
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.
When drop is implemented, it can be moved partially.
self.pending_reads.clear_uncommitted(&self.tag, term); | ||
} | ||
if ready.ss.is_some() { | ||
let term = self.term(); |
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 not clear for leader before?
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.
When the peer is not leader before, there will be no uncommitted in pending reads. Remove the check just make code more simple.
return true; | ||
} | ||
return false; | ||
return match self.block { |
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.
Can we add some annotations in this file, it's hard for me to understand?
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.
Sure, but not related to this pr.
LGTM |
Callback of read index isn't cleared when the peer is destroyed, which will lead to dead lock.
@siddontang @zhangjinpeng1987 @hhkbp2 PTAL