Skip to content
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

direct local reader for raw read #4222

Merged
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

fix bug about _timer

Signed-off-by: fredchenbj <cfworking@163.com>
  • Loading branch information...
fredchenbj committed Feb 15, 2019
commit d19f7ce2112a407d32667872d06711ddf6417950
@@ -1140,6 +1140,7 @@ impl<E: Engine> Storage<E> {
})
})
.and_then(move |res| {
_timer.observe_duration();
let r = tx.send(res);

This comment has been minimized.

Copy link
@siddontang

siddontang Feb 16, 2019

Contributor

em, seem it must be sent successfully. do we need to panic here?

/cc @overvenus

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Feb 16, 2019

Member

Agree, or the command will be blocked for ever.

This comment has been minimized.

Copy link
@ngaut

ngaut Feb 17, 2019

Member

Does it really helpful to add a timer metric here, The send method of tx should be fast or blocked, other metrics could be used to reflect the timer metric. And we can remove the metric in another PR.

This comment has been minimized.

Copy link
@overvenus

overvenus Feb 17, 2019

Contributor

seem it must be sent successfully. do we need to panic here?

By panicking, we may unable to shutdown tikv gracefully.


or the command will be blocked for ever.

It's a raw get, I think clients will timeout.


Does it really helpful to add a timer metric here, The send method of tx should be fast or blocked, other metrics could be used to reflect the timer metric. And we can remove the metric in another PR.

It does not record the send method, instead it records from L1108 to L1143, means how long it will spend for taking a snapshot and perform a raw get op.

This conversation was marked as resolved by breeswish

This comment has been minimized.

Copy link
@breeswish

breeswish Feb 19, 2019

Member

This error should be passed to the outside (i.e. as the Error for this function)

This comment has been minimized.

Copy link
@fredchenbj

fredchenbj Feb 19, 2019

Author Contributor

This error is forgotten, for here is the callback of async_snapshot. So maybe only could pass result to the outside through one::channel, and all errors are processed in the end of this function, i.e. future::result(res)....

This comment has been minimized.

Copy link
@breeswish
if r.is_err() {
warn!("future callback: Failed to send result to the future rx, discarded.");

This comment has been minimized.

Copy link
@siddontang

siddontang Feb 20, 2019

Contributor

any case that the sending can fail?

@breeswish

@@ -1210,6 +1211,7 @@ impl<E: Engine> Storage<E> {
Ok(result)
})
.and_then(move |res| {
_timer.observe_duration();
let r = tx.send(res);
if r.is_err() {
warn!("future_callback: Failed to send result to the future rx, discarded.");
@@ -1517,6 +1519,7 @@ impl<E: Engine> Storage<E> {
result
})
.and_then(move |res| {
_timer.observe_duration();
let r = tx.send(res);
if r.is_err() {
warn!("future_callback: Failed to send result to the future rx, discarded.");
@@ -1650,6 +1653,7 @@ impl<E: Engine> Storage<E> {
Ok(result)
})
.and_then(move |res| {
_timer.observe_duration();
let r = tx.send(res);
if r.is_err() {
warn!("future_callback: Failed to send result to the future rx, discarded.");
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.