Remove ErrorRequestHandler #4303
Conversation
Signed-off-by: Breezewish <[email protected]>
Signed-off-by: Breezewish <[email protected]>
Signed-off-by: Breezewish <[email protected]>
Signed-off-by: Breezewish <[email protected]>
Signed-off-by: Breezewish <[email protected]>
|
@AndreMouche @BusyJay @hicqu PTAL, thanks! |
|
do we need to construct an error that exceeds the 1000 limit for the protobuf? |
| @@ -249,6 +250,14 @@ impl Engine for RocksEngine { | |||
| } | |||
|
|
|||
| fn async_snapshot(&self, _: &Context, cb: Callback<Self::Snap>) -> Result<()> { | |||
| fail_point!("rockskv_async_snapshot", |_| Err(box_err!( | |||
There was a problem hiding this comment.
We have one for raftkv async snapshot. So let's add one for rocksdb kv as well. In coprocessor tests only rocksdb engine is used.
| .future_execute(priority, move |ctxd| { | ||
| tracker.attach_ctxd(ctxd); | ||
| Self::handle_unary_request_impl(engine, tracker, handler_builder) | ||
| }) | ||
| .map_err(|_| Error::Full) |
There was a problem hiding this comment.
I don't think it's correct anymore. What if the returned error is a Error::Region?
There was a problem hiding this comment.
@BusyJay future_execute itself returns Result<impl Future>. The map_err here only maps the out most Result, not the inner one. For the out most result, it can be only caused by read pool full. Does it solve your concern?
There was a problem hiding this comment.
Why not impl From for the error returned by read_pool? So no one has the concern anymore.
There was a problem hiding this comment.
This solution was not accepted at that time. Reviewers said that it is not good to mix the sync part into the async interface.
|
@BusyJay @hicqu @AndreMouche PTAL, thanks! |
|
@hicqu @overvenus @AndreMouche PTAL |
|
@hicqu PTAL |
Signed-off-by: Breezewish <[email protected]>
|
/run-integration-tests |
|
LGTM. |
Signed-off-by: Breezewish <[email protected]>
Signed-off-by: Breezewish <[email protected]>
Signed-off-by: Breezewish <[email protected]>
What have you changed? (mandatory)
Previously, we:
fn parse_request)ErrorRequestHandler. If parse request succeeded, create a correspondingRequestHandler. Anyway, there will be aRequestHandler(let's call itR).fn handle_unary_request)future::ok(AResponseOfFullError). If read pool is not full, continue.Rto get a result.This causes #4293 because the process "Get a snapshot" requires a correctly parsed request in real world.
Now we:
fn parse_request)Rto a correctly parsedRequestHandlerto get a result.What are the type of the changes? (mandatory)
How has this PR been tested? (mandatory)
Newly added some fail point tests.