-
Notifications
You must be signed in to change notification settings - Fork 1.7k
rpc_tester: introduce rpc_streaming job based on streaming API #3173
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
base: master
Are you sure you want to change the base?
rpc_tester: introduce rpc_streaming job based on streaming API #3173
Conversation
a938035 to
f557f81
Compare
ChangelogRequested changes
Additional changes
Benchmark after changes |
apps/rpc_tester/rpc_tester.cc
Outdated
| , _ccfg(ccfg) | ||
| , _rpc(rpc) | ||
| , _stop(std::chrono::steady_clock::now() + _cfg.duration) | ||
| , _payload_size_bytes(_cfg.payload), |
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.
Nit: comma in the next line
apps/rpc_tester/rpc_tester.cc
Outdated
| , _payload_size_bytes(_cfg.payload), | ||
| _payload(_cfg.payload / sizeof(payload_t::value_type), 0) { | ||
| if (_cfg.verb == "bidirectional") { | ||
| _call = [this] (unsigned worker_id, const payload_t& payload) { |
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.
Nit: We usually do the following indentation:
name()
: _a()
, _b()
{
do_sth();
}
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.
(Twice before params initialization, once before instructions)
apps/rpc_tester/rpc_tester.cc
Outdated
|
|
||
| private: | ||
| future<> stream_data(rpc::sink<payload_t> sink, const payload_t& payload) { | ||
| return do_with(std::move(sink), [this, &payload] (rpc::sink<payload_t>& sink) { |
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.
Why not a coroutine here?
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.
to flush and close the sink even on error. Can it be done easier?
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.
you can use try-catch clause
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 about:
template<typename T>
static future<> flush_and_close(rpc::sink<T>& sink){
try {
co_await sink.flush();
} catch (...) {
}
try {
co_await sink.close();
} catch (...) {
}
}
future<> stream_data(rpc::sink<payload_t> sink, const payload_t& payload) {
auto d = defer([&sink] () noexcept {
seastar::async([s = std::move(sink)] () mutable {
flush_and_close(s).get();
}).handle_exception([](auto) {});
});
while (std::chrono::steady_clock::now() <= _stop) {
++_total_messages;
co_await sink(payload);
if (_cfg.sleep_time) {
co_await seastar::sleep(std::chrono::duration_cast<std::chrono::nanoseconds>(*_cfg.sleep_time));
}
}
}is it viable?
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.
oh, we're ignoring future here (::async)...
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.
.close() shouldn't cause exceptions if you flush() before, so the second try is unnecessary.
Creating a thread should be reserved for top-level operations so their numbers can be kept in check (threads are heavyweight).
You can keep it simple - a try/catch around the outer loop + flush, capturing the exception, then a close, then returning the exception if any.
See utils/closeable.hh, with_closeable.
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.
Thanks, we will check it out
apps/rpc_tester/rpc_tester.cc
Outdated
| static future<> process_bi_source(rpc::source<payload_t> source, rpc::sink<payload_t> sink) { | ||
| uint64_t total_messages = 0, total_payload = 0; | ||
|
|
||
| co_await repeat([&source, &sink, &total_messages, &total_payload] { |
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.
This should be changed to while loop and co_awaits
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 about #3173 (comment) ?
We tried to keep continuations in the loop for streaming/consuming the data, and co-routines for everything else
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.
it may be a continuation chain but then you don't need to co_await repeat as it doesn't give much
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.
@xemul @avikivity I'm not sure about this one. Which approach do you suggest here (as it's in the main path):
- The current one: couroutine in the outer scope + repeat
- Only continuation chain in the whole
process_bi_source - Only coroutines?
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.
For an rpc tester, there's no reason not to coroutines everywhere, it's the cleanest option and the small performance hit won't matter.
apps/rpc_tester/rpc_tester.cc
Outdated
| static future<> process_uni_source(rpc::source<payload_t> source, rpc::sink<uint64_t> sink) { | ||
| uint64_t total_messages = 0, total_payload = 0; | ||
|
|
||
| co_await repeat([&source, &sink, &total_messages, &total_payload] { |
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.
Same here, if you are using coroutines, use their whole potential
|
Are those results repeatable? The dependency on shard count looks random. |
they tend to have some deviations, but we suspect it's because of server's load (not only we're using it). We can run benchmarks and plot it with standard deviation. Would you want to see such charts? |
Prefer to see known stable results. |
What do you mean by "dependency on shard count"? Do you mean that shards with different ids (in the graphs above from 0 to 7) differ among each other for the same run of |
We run the tests on a server that is shared among other employees, so the most we can do for stability of the results is to run the tests a couple of times and calculate standard deviation (to minimize chance of results being impacted by other users of the server). |
This commit extends the rpc_tester with rpc_streaming job that uses rpc::sink<> and rpc::source<> to stream data between the client and the server. We introduce verb STREAM_BIDIRECTIONAL: - Streaming is bidirectional - the client streams the payload_t to the server, the server streams to the client received data back on each read portion of data. - The client sends the configured number of bytes to the server throught rpc::sink, while simultaneously reading from the rpc::source received from the server. The server, for each stream, starts a background job to read data from it.
We introduce an unidirectional streaming to the rpc_streaming job. Client streams the configured number of bytes, while server sums the total data length and keeps the stream to client open, not sending anything. After client closes the stream to server, server closes the one to the client. Client waits for EOF on the stream from the server, then finishes the job.
f557f81 to
cca65a3
Compare
ChangelogChanges in code
Different graphsWe changed the way we generate graphs - now we ran each test three times and calculated the standard deviation (plotted in black). Benchmark after changes |
| } | ||
|
|
||
| try { | ||
| co_await sink.flush(); |
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.
Those try-catches don't look best. Try this instead:
auto flush_future = co_await coroutine::as_future(sink.flush());
if (flush_future.failed() && !error) [[unlikely]] {
error = f.get_exception();
}
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.
Note: flush_future.failed() is first on purpose. It would complain if future failed and it wasn't checked
| } | ||
|
|
||
| try { | ||
| co_await sink.flush(); |
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.
Ditto
| } catch (...) { | ||
| } | ||
| try { | ||
| co_await sink.close(); |
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.
And here










In this PR we introduce new
rpc_streamingjob - withSTREAM_UNIDIRECTIONALandSTREAM_BIDIRECTIONALverbs.Overview
The streaming job uses Seastar’s RPC streaming API by continuously sending payloads from workers to the server, optionally receiving responses on a return stream. It is intended for measuring end-to-end throughput.
Similar to the
rpcjob'srpc_verb::writebehavior but replaces per-payload RPC calls with the streaming API and doesn't wait for response from the server between writes.Metrics
payload_tthat was put by workers._total_messages * _payload_size_bytes / _total_duration.count(), measured inkB/s.messages / _total_duration.count()Benchmark
Below you can find a simple benchmark that was run on
potwor2for 30s machine with:cpuset=0-7cpuset=8-15All configs:
suite.yaml
rpc_streaming::bidirectional
rpc_streaming::unidirectional
rpc::write
rpc::write doesn't produce throughput metric
Client
In
clientmode app:rpc::sink<payload_t>, send it to the server and receives a correspondingsource<uint64_t>Server
For each
STREAM_UNIDIRECTIONALorSTREAM_BIDIRECTIONALcall:rpc::sink<uint64_t>rpc::source<payload_t>received from the client.rpc::source<uint64_t>associated with the createdrpc::sink<uint64_t>, used for streaming back the number of bytes received.For the bidirectional verb, the server sends back, on its stream, the number of bytes received for each chunk of data.
After client closes their
rpc::sink<payload_t>, the server writes the final total number of bytes into correspondingrpc::sink<uint64_t>and then closes it.