Internal Review: QUIC send back errors backport 1.14.12 #20
Conversation
… in connection cache
…ge incase of errors.
| @@ -4242,27 +4245,51 @@ impl Bank { | |||
| .filter_map(|(index, res)| match res { | |||
| // following are retryable errors | |||
| Err(TransactionError::AccountInUse) => { | |||
| bidirection_reply_service.send_message( | |||
There was a problem hiding this comment.
why do you need to trigger this from two places?
There was a problem hiding this comment.
Because these types of errors is not tracked in qos_service.rs.
There was a problem hiding this comment.
oh i see, so they are exhaustive and non-overlapping?
There was a problem hiding this comment.
Normally yes but still will confirm it while doing final review.
|
|
||
| pub fn get_signature_from_packet(packet: &Packet) -> Option<Signature> { | ||
| // add instruction signature for message | ||
| match packet.data(1..65) { |
There was a problem hiding this comment.
for a real PR, it would be better to make this a bit more robust, for testing lite rpc it's probably ok
| match task { | ||
| Ok(message) => { | ||
| let serialized_message = serialize(&message).unwrap(); | ||
| let _ =send_stream.write(serialized_message.as_slice()).await; |
There was a problem hiding this comment.
not sure this is ideal, do you know if there's any buffering happening inside send_stream.write?
It's not very efficient for the network, when you send a lot of messages with 68 byte length, better to buffer at least to a few 100 bytes.
please add some metrics here, so we get an idea of how many messages / s this processes
There was a problem hiding this comment.
Ok sending in batchs of size 16
There was a problem hiding this comment.
dont think batching is important, actually think that's premature optimization, just important to measure how often it is called
There was a problem hiding this comment.
I did some testing and to correctly handle the replies in reply handler I had to make the QuicReplyMessage of fixed length of 200 bytes. On the handle side I read the chunks and when the chunk is greater than 200 bytes I get to know that we got a reply.
This is different than the quic connection in the TPU because when client sends all the messages to the tpu server we close the connection so we know that all packets are transfered. On contrary we keep the send channel open on server side until it is closed by the client. So in this case client will never know when it will get the last message.
| } | ||
| Err(e) => { | ||
| // recv error | ||
| warn!("got {} on quic bidirectional channel", e.to_string()); |
There was a problem hiding this comment.
better use metrics, not sure how often this error can be triggered in production
|
@mschneider will add the metrics after testing the PR. |
…ments, properly closing send and recv streams
…d channels for transactions
No description provided.