Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13883 )
Change subject: IMPALA-8779, IMPALA-8780: RowBatchQueue re-factoring and BufferedPRS impl ...................................................................... Patch Set 15: Code-Review+1 (7 comments) This mostly looks good, I had a few minor comments but LGTM http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.h File be/src/exec/buffered-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.h@21 PS15, Line 21: #include "runtime/deque-row-batch-queue.h" I'd prefer a forward declaration of this in the header and moving the include to the .cc if possible (just trying to reduce the web of include dependencies) http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.h@33 PS15, Line 33: eiher either http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.h@80 PS15, Line 80: is_full_ I think the polarity of this name doesn't match the other condition variables, since it's signalled when the queue becomes not full. I'd recommend naming it something like batch_queue_has_capacity_. http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@123 PS11, Line 123: // If the query was cancelled while the sink was waiting for rows to become available, : // or if the query was cancelled before the current call to GetNext, set eos and then : // return. The queue could be empty if the sink was closed > Makes sense. CHECK() seems a bit an overkill. Sorry for the bad suggestion. Oh no, I think it's generally a good idea to fail hard, just that specific scenario was a concern. http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.cc@38 PS15, Line 38: if (batch->num_rows() == 0) { nit: if statement could be one line http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.cc@52 PS15, Line 52: batch->DeepCopyTo(output_batch.get()); You could Reset() batch at this point to free up any resources associated. http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.cc@144 PS15, Line 144: if (*eos) consumer_eos_.NotifyOne(); I found the mix of NotifyOne()/NotifyAll() is mildly confusing. I think either is actually ok in all contexts, since at most 1 thread can wait on each condition variable, which means that NotifyOne() is correct and NotifyAll() does not result in unnecessary wakeups. So we could consider just standardising on one. I think I understand the reasoning because which is used where, but initially I was wondering if there was something subtle going on. Ok to leave code as is, but might help to clarify in the comments about when NotifyOne/NotifyAll is called. -- 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: 15 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: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 30 Jul 2019 20:30:07 +0000 Gerrit-HasComments: Yes
