grpc-benchmark: Add benchmark client and local runner#2737
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the benchmark client and worker client execution logic for the gRPC benchmark suite, including support for collecting latency statistics using histograms and running smoke tests. The feedback highlights several important improvements: preventing empty authority overrides from failing gRPC requests, handling empty histograms to avoid reporting incorrect minimum latencies, adding a small delay in the unary call retry loop to prevent 100% CPU usage on failures, and replacing an unsafe .unwrap() with proper error handling when accessing the benchmark client.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a benchmark tool (bench) and implements the client-side benchmarking logic (BenchmarkClient) along with its integration into the worker service. It also updates the benchmark server to support dynamic port binding. The review feedback highlights two critical issues in client.rs: first, stale histogram reports from previous timed-out requests can corrupt benchmark statistics, which can be resolved by draining the receiver channel; second, if a streaming task fails, it exits prematurely, causing the stats aggregation to hang indefinitely. Re-establishing the stream on failure is recommended to prevent this hang.
e39fe6e to
8dfa7d9
Compare
Contributes to: #2666
This PR introduces the benchmarking client. It also adds a local runner binary that exercises the worker, server, and client within a single process, making it easy to run benchmarks locally.
Similar to gRPC-Java, the client uses the
hdrhistogramcrate for its histogram implementation. Because the Rust crate does not support a thread-safe, lock-free implementation natively, the client leverages atokio::sync::watchchannel and atokio::sync::mpscchannel to coordinate between the worker task and the client RPC tasks when collecting stats. This design avoids the need to acquire a mutex lock when reporting the latency of each RPC.