[GitHub] [incubator-doris] imay commented on a change in pull request #4237: [BUG] Using attachement strategy of brpc to send packet with big size.

[GitBox <gi...@apache.org>]

imay commented on a change in pull request #4237:
URL: https://github.com/apache/incubator-doris/pull/4237#discussion_r464391138



##########
File path: be/src/common/config.h
##########
@@ -519,6 +519,8 @@ namespace config {
     CONF_Int64(brpc_max_body_size, "209715200");
     // Max unwritten bytes in each socket, if the limit is reached, Socket.Write fails with EOVERCROWDED
     CONF_Int64(brpc_socket_max_unwritten_bytes, "67108864");
+    // If batch size large than brpc_attachment_threashold, use attachment instead

Review comment:
       ```suggestion
       // If batch size is larger than brpc_attachment_threshold, use attachment instead
   ```
   I have a question, why not use threshold directly for all the packets? 

##########
File path: be/src/runtime/data_stream_sender.cpp
##########
@@ -270,12 +277,7 @@ Status DataStreamSender::Channel::add_row(TupleRow* row) {
 }
 
 Status DataStreamSender::Channel::send_current_batch(bool eos) {
-    {
-        SCOPED_TIMER(_parent->_serialize_batch_timer);
-        int uncompressed_bytes = _batch->serialize(&_pb_batch);
-        COUNTER_UPDATE(_parent->_bytes_sent_counter, RowBatch::get_batch_size(_pb_batch));
-        COUNTER_UPDATE(_parent->_uncompressed_bytes_counter, uncompressed_bytes);
-    }
+    _parent->serialize_batch(_batch.get(), &_pb_batch, 1);

Review comment:
       why delete origin timer and counter?

##########
File path: be/src/runtime/data_stream_sender.cpp
##########
@@ -227,7 +227,14 @@ Status DataStreamSender::Channel::send_batch(PRowBatch* batch, bool eos) {
     }
 
     _brpc_request.set_eos(eos);
-    if (batch != nullptr) {
+    if (batch != nullptr && RowBatch::get_batch_size(*batch) > config::brpc_socket_max_unwritten_bytes) {
+        std::string* tuple_data_to_attachment = batch->release_tuple_data();

Review comment:
       Will this variable will cause memory leak?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org