[server] proper handling of batch errors and mixed calls #917
Conversation
niklasad1
force-pushed
the
na-batch-errors
branch
from
a889704
to
3d10ef8
Compare
niklasad1
force-pushed
the
na-batch-errors
branch
from
450f2b9
to
d6ba724
Compare
| Err(batch_err) => batch_err, | ||
| }; | ||
| } | ||
| if let Ok(batch) = serde_json::from_slice::<Vec<&JsonRawValue>>(&data) { |
There was a problem hiding this comment.
parsing the request like this is slower than parsing directly Vec<Request> which is the most common usecase
overall, it looks to similar to master which I'm happy with considering that I changed the responses to be in order now.
There was a problem hiding this comment.
consider that my machine is a bit spurious for the benches:
group batch-ordered master
----- ------------- ------
async/http_batch_requests/fast_call/10 1.26 57.9±8.16µs 168.6 KElem/sec 1.00 46.0±3.16µs 212.4 KElem/sec
async/http_batch_requests/fast_call/100 1.00 281.0±62.15µs 347.6 KElem/sec 1.20 337.5±31.42µs 289.4 KElem/sec
async/http_batch_requests/fast_call/2 1.32 43.6±9.62µs 44.8 KElem/sec 1.00 33.1±2.01µs 59.0 KElem/sec
async/http_batch_requests/fast_call/5 1.56 62.7±16.28µs 77.8 KElem/sec 1.00 40.2±2.96µs 121.5 KElem/sec
async/http_batch_requests/fast_call/50 1.04 182.8±44.29µs 267.2 KElem/sec 1.00 175.3±48.17µs 278.5 KElem/sec
async/ws_batch_requests/fast_call/10 1.00 82.2±9.43µs 118.8 KElem/sec 1.03 84.6±3.72µs 115.4 KElem/sec
async/ws_batch_requests/fast_call/100 1.00 305.9±37.64µs 319.3 KElem/sec 1.18 359.7±19.63µs 271.5 KElem/sec
async/ws_batch_requests/fast_call/2 1.00 42.1±3.17µs 46.4 KElem/sec 1.23 51.8±3.90µs 37.7 KElem/sec
async/ws_batch_requests/fast_call/5 1.00 30.9±1.19µs 158.0 KElem/sec 1.75 54.2±11.31µs 90.2 KElem/sec
async/ws_batch_requests/fast_call/50 1.00 185.8±22.98µs 262.9 KElem/sec 1.23 228.3±14.44µs 213.9 KElem/sec
sync/http_batch_requests/fast_call/10 1.00 72.9±12.58µs 134.0 KElem/sec 1.15 83.9±3.51µs 116.3 KElem/sec
sync/http_batch_requests/fast_call/100 1.14 335.2±42.17µs 291.4 KElem/sec 1.00 292.8±32.85µs 333.5 KElem/sec
sync/http_batch_requests/fast_call/2 1.28 45.0±9.56µs 43.4 KElem/sec 1.00 35.0±4.86µs 55.8 KElem/sec
sync/http_batch_requests/fast_call/5 1.15 49.3±9.73µs 99.1 KElem/sec 1.00 42.7±7.20µs 114.4 KElem/sec
sync/http_batch_requests/fast_call/50 1.00 150.9±41.80µs 323.6 KElem/sec 1.05 159.0±4.82µs 307.2 KElem/sec
sync/ws_batch_requests/fast_call/10 1.00 48.3±14.22µs 202.2 KElem/sec 1.27 61.4±22.71µs 159.1 KElem/sec
sync/ws_batch_requests/fast_call/100 1.00 272.4±46.48µs 358.5 KElem/sec 1.26 343.6±22.20µs 284.2 KElem/sec
sync/ws_batch_requests/fast_call/2 1.05 25.8±1.33µs 75.7 KElem/sec 1.00 24.5±0.72µs 79.6 KElem/sec
sync/ws_batch_requests/fast_call/5 1.04 30.9±1.28µs 158.3 KElem/sec 1.00 29.8±0.85µs 163.8 KElem/sec
sync/ws_batch_requests/fast_call/50 1.03 210.7±18.53µs 231.7 KElem/sec 1.00 205.2±10.41µs 237.9 KElem/sec
| let mut got_notif = false; | ||
| let mut batch_response = BatchResponseBuilder::new_with_limit(call.max_response_body_size as usize); | ||
|
|
||
| let mut pending_calls: FuturesOrdered<_> = batch |
There was a problem hiding this comment.
Is there any way to avoid duplicating this logic I wonder? Like a single function whose job it is to take a batch request, run some provided function on each valid item and aggregate the results in the right way.
Maybe more complicated than it's worth, but I wonder how much we could separate out the logic that doesn't care specifically about ws/http and only cares about the formatting and such of messages I guess.
(Doesn't need to be related to this PR; just a general thought!)
There was a problem hiding this comment.
yeah, the only thing that's different is the call type which needs refactoring to work.
let's do it in another PR eventually :)
| tx_log_from_str(&response.result, max_log_length); | ||
| logger.on_response(&response.result, request_start, TransportProtocol::WebSocket); | ||
| let _ = sink.send_raw(response.result); | ||
| if let Some(response) = response { |
There was a problem hiding this comment.
Ok, so this basically catches the case when every request is a notification; gotcha!
Builds on top of #896, kudos to @polachok for starting this.
As he hasn't responded on my feedback then I fixed it myself.
So before we just checked if the batches was
Vec<Request>orVec<Notificationwhich doesn't work for errors and batches with mixed calls and notifications which this fixes.In addition I removed the empty message that are we sent on batches with only notifications on
WSfor HTTP we are still doing it to ACK to HTTP request itself.