test/raftstore: add random latency filter for simulation transport #1120
Conversation
| } | ||
|
|
||
| impl Filter<RaftMessage> for RandomLatencyFilter { | ||
| fn before(&self, msgs: &mut Vec<RaftMessage>) { |
There was a problem hiding this comment.
If msgs has only one message, we will return error for this send, and raft will re-send the message again. So this may be not for delay, but duplicated.
There was a problem hiding this comment.
A filter result type is added to let RandomLatencyFilter be capable to ignore this message dropped error. PTAL.
| @@ -288,7 +288,7 @@ struct StaleSnap { | |||
| } | |||
|
|
|||
| impl Filter<RaftMessage> for Arc<StaleSnap> { | |||
| fn before(&self, msgs: &mut Vec<RaftMessage>) { | |||
| fn before(&self, msgs: &mut Vec<RaftMessage>) -> FilterResult { | |||
There was a problem hiding this comment.
we can return Result<()> directly and no need to check msg.empty() later.
In the delay filter, we can return ok, and for other filters, we can use a function like
fn check_droppable_err(msg:&Vec<RaftMessage>) -> Result<()> {
if msg.empty() {
return DropableError
} else {
return ok
}
}
There was a problem hiding this comment.
Er, what's the advantage of use Result<()> instead of FilterResult?
There was a problem hiding this comment.
I am confused with FilterResult, and in fact, I don't think it is FilterResult, but only returns a result and let outer check, so why not using Result directly? And we can ignore checking msg.empty() too.
There was a problem hiding this comment.
Maybe FilterResult is a bad name. But it's a helpful pattern for a filter to return a flag, and the SimulateTransport acts differently according to the returned flag, such as to skip checking whether the messages is dropped, to shortcut the following filters, etc. In the previous comment of the definition of Filter::before()
https://github.com/pingcap/tikv/blob/master/tests/raftstore/transport_simulate.rs#L48
(this comment is obsolete since before does not return bool any more), this pattern is used by returning a bool. A bool is not good for naming and future extension. An enum is better.
There was a problem hiding this comment.
- Result is also an enum.
- Now you return the result for every Filter::before, so the filter can check whether return ok or error itself, if error,
sendreturns error, we don't need to check the message is empty and whether we should return DroppableError or not again. - If the filter meets an error but want to go on, it can return ok.
- We can check error if we really want to filter some cases later, but I think we don't need this.
| if res.is_err() { | ||
| break; | ||
| } | ||
| for msg in msgs { |
There was a problem hiding this comment.
missing check res error here.
|
LGTM |
|
PTAL |
| let mut to_delay = vec![]; | ||
| let mut delayed_msgs = self.delayed_msgs.lock().unwrap(); | ||
| // check whether to send those messages which are delayed previouly | ||
| for m in delayed_msgs.drain(..) { |
There was a problem hiding this comment.
for m in delayed_msgs.drain(..).chain(msgs)
| if self.stale.load(Ordering::Relaxed) { | ||
| return; | ||
| return check_messages(msgs); |
There was a problem hiding this comment.
check_messages() will return Ok(()) if msgs is not empty. An error returned by before will be delivered to SimulateTransport.
| } | ||
|
|
||
|
|
||
| pub struct RandomLatencyFilterFactory { |
There was a problem hiding this comment.
I think use CloneFilterFactory is enough.
There was a problem hiding this comment.
The problem is that Mutex does not implement Clone.
There was a problem hiding this comment.
Why not impl Clone for RandomLatencyFilter?
|
LGTM |
hhkbp2
deleted the
hhkbp2/add-random-latency-filter-for-simulation-transport
branch
Signed-off-by: Ping Yu <yuping@pingcap.com>
For issue #963
PTAL @siddontang @BusyJay