Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21579 )
Change subject: POC: Add queuing to KrpcDataStreamSender ...................................................................... Patch Set 12: (12 comments) http://gerrit.cloudera.org:8080/#/c/21579/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21579/12//COMMIT_MSG@12 PS12, Line 12: 2. in-fligh batch (with current active RPC) Spell check on the commit message would catch a few issues. http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.h File be/src/runtime/krpc-data-stream-sender.h: http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.h@211 PS12, Line 211: KrpcDataStreamSender* parent_ = nullptr; Instead of holding an extra pointer, could be a GetParent() method that calls channel_.GetParent(). Not sure which is better for codegen. http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.h@237 PS12, Line 237: class OutboundQueue { nit: private class could be defined separately, like a separate header file only included by krpc-data-stream-sender.cc. http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.h@263 PS12, Line 263: Status WaitTillEmpty(); nit: Til is a shortening of Until, which only has 1 'l'. So I'd do WaitTilEmpty. http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.h@300 PS12, Line 300: std::vector<Channel*> channels_; Consider https://www.boost.org/doc/libs/1_74_0/doc/html/boost/container/static_vector.html. Or at least call 'reserve' on construction. Same for idle_channels_. http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.h@317 PS12, Line 317: std::list<QueuedBatch> queue_; If we're trying to minimize memory allocation/deallocation, this might not be the best data structure. A deque uses a bit more memory around boundaries, but does fewer allocations and has better cache performance for processing sequential blocks. http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.h@507 PS12, Line 507: std::list<std::unique_ptr<OutboundRowBatch>> free_batch_pool_; Use deque (or queue) here too. http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.cc@475 PS12, Line 475: Status start_err; I think this section would be helpful to move to a new function with a descriptive name. http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.cc@918 PS12, Line 918: if (partition_type_ == TPartitionType::UNPARTITIONED) { This and the other partition_type_ checks would make more sense as a switch statement. http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.cc@920 PS12, Line 920: queues_.emplace_back(new OutboundQueue(channels_, this)); queues_.reserve first, or use static_vector. http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.cc@1189 PS12, Line 1189: RETURN_IF_ERROR(queues_[0]->Add(&serialization_batch)); Should we DCHECK_EQ(queues_.size(), 1)? http://gerrit.cloudera.org:8080/#/c/21579/12/be/src/runtime/krpc-data-stream-sender.cc@1483 PS12, Line 1483: queue_.emplace_back(std::move(qb)); QueuedBatch can be constructed directly in emplace_back like queue_.emplace_back({batch->release(), channels_.size() - closed_channel_count_}); -- To view, visit http://gerrit.cloudera.org:8080/21579 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe3d01fada31f1b48de6c1403e7d2955114d8078 Gerrit-Change-Number: 21579 Gerrit-PatchSet: 12 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Wed, 06 May 2026 18:11:43 +0000 Gerrit-HasComments: Yes
