-
Notifications
You must be signed in to change notification settings - Fork 253
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
benchmark: fix shutdown failure #127
Conversation
benchmark/src/client.rs
Outdated
@@ -143,6 +143,17 @@ impl<B: Backoff> ExecutorContext<B> { | |||
} | |||
} | |||
|
|||
// Since impl trait is not stable yet, implement this as a function is impossible without box. | |||
macro_rules! spawn { |
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.
What's the difference between the fn spawn()
above?
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.
client
can be different types.
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.
Could we remove the above fn spawn()
?
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
PTAL @AndreMouche |
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.
Rest LGTM
benchmark/src/bench.rs
Outdated
@@ -26,16 +30,28 @@ fn gen_resp(req: SimpleRequest) -> SimpleResponse { | |||
resp | |||
} | |||
|
|||
|
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.
redundant empty line?
benchmark/src/client.rs
Outdated
@@ -143,6 +143,17 @@ impl<B: Backoff> ExecutorContext<B> { | |||
} | |||
} | |||
|
|||
// Since impl trait is not stable yet, implement this as a function is impossible without box. | |||
macro_rules! spawn { |
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.
Could we remove the above fn spawn()
?
3816671
to
6ec3281
Compare
ping |
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
This pr fixes an hang-up-forever issue during benchmark.
To add all the stats 1.7.2 introduces, we need a public API and compatible rust-protobuf. See grpc/grpc#13705 and stepancheg/rust-protobuf#260.
/cc #107 #118
After this pr is merged, I think we are ready to release 0.2.0.