Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8023 )
Change subject: IMPALA-4856: Port data stream service to KRPC ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc File be/src/runtime/krpc-data-stream-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc@198 PS1, Line 198: AddData Isn't the point of the deserialize pool to deserialize the payload early? Here, we're just calling AddData() on the payloads for early senders after the corresponding receiver has been created. http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc@165 PS1, Line 165: Either we'll consume a row batch from batch_queue_, or it's empty Shouldn't there always be something in the batch_queue_ if there's something in the blocked_senders_ list? Since we fill the blocked_senders_ only if the queue is at its limit. And we also logically service the batches from batch_queue_ first before servicing the batches from the blocked_senders_ list. http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc@166 PS1, Line 166: There is a window Just to make things clearer, could you specify what there's a window for? http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc@225 PS1, Line 225: There is a problem here. When we release lock_here, an arbitrary number of senders could call AddBatch(), and all their batches would get enqueued even though the ExceedsLimit() would be true. This breaks the guarantee of the queue not being over committed more than a single batch. http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc@284 PS1, Line 284: for (const auto& queue_entry: batch_queue_) delete queue_entry.second; batch_queue_.clear() ? -- To view, visit http://gerrit.cloudera.org:8080/8023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic0b8c1e50678da66ab1547d16530f88b323ed8c1 Gerrit-Change-Number: 8023 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Tue, 26 Sep 2017 18:11:25 +0000 Gerrit-HasComments: Yes
