Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13883 )
Change subject: IMPALA-8656: Add RowBatchQueue interface and BufferedPlanRootSink impl ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/exec/buffered-plan-root-sink.cc@44 PS4, Line 44: if Should this be while() ? Otherwise, I don't see how you will add the batch into the queue after waking up from is_full_ below. http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/exec/buffered-plan-root-sink.cc@45 PS4, Line 45: is_full_.Wait(l); Looking at this code again, the thread will block when the queue is full and unblock when a row batch is dequeued in GetNext() or if the sink is cancelled. So, why cannot we just use the existing blocking queue for it ? I could be missing something though. For cancellation, we can simply call Shutdown() on this queue and the thread will immediately be unblocked. http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/runtime/non-blocking-row-batch-queue.cc File be/src/runtime/non-blocking-row-batch-queue.cc: http://gerrit.cloudera.org:8080/#/c/13883/4/be/src/runtime/non-blocking-row-batch-queue.cc@44 PS4, Line 44: unique_ptr<RowBatch> result = move(batch_queue_.front()); : batch_queue_.pop(); Does it assume this function is called only when the queue is not empty ? -- To view, visit http://gerrit.cloudera.org:8080/13883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b1bb4b9c6f6e92c70e8fbee6ccdf48c2f85b7be Gerrit-Change-Number: 13883 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 23 Jul 2019 06:59:31 +0000 Gerrit-HasComments: Yes
