Sahil Takiar 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 16: (10 comments) 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 "util/condition-variable.h" > I'd prefer a forward declaration of this in the header and moving the inclu Done http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.h@33 PS15, Line 33: s unt > either Done http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.h@80 PS15, Line 80: produce > I think the polarity of this name doesn't match the other condition variabl Done 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: while (batch_queue_->IsEmpty() && sender_state_ == SenderState::ROWS_PENDING : && !state->is_cancelled()) { : rows_available_.Wait(l); > Oh no, I think it's generally a good idea to fail hard, just that specific Done http://gerrit.cloudera.org:8080/#/c/13883/11/be/src/exec/buffered-plan-root-sink.cc@127 PS11, Line 127: > Added the call to is_full_.NotifyOne(), agree the early return isn't ideal. Done 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 the batch is empty, we have nothing to do so just return Status::OK(). > nit: if statement could be one line Done http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.cc@52 PS15, Line 52: > You could Reset() batch at this point to free up any resources associated. The FragmentInstanceState calls Reset() on each batch after it is passed to ::Send - https://github.com/apache/impala/blob/master/be/src/runtime/fragment-instance-state.cc#L365 http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/buffered-plan-root-sink.cc@144 PS15, Line 144: RETURN_IF_ERROR( > I found the mix of NotifyOne()/NotifyAll() is mildly confusing. I think eit Added some comments to clarify this. The suggestion to use NotifyAll() was from prior review comments to use NotifyAll() in Close and FlushFinal to be extra sure that all threads are awoken. http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/plan-root-sink.h File be/src/exec/plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/exec/plan-root-sink.h@94 PS15, Line 94: Updat > UpdateAndCheck ? Done http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/runtime/blocking-row-batch-queue.h File be/src/runtime/blocking-row-batch-queue.h: http://gerrit.cloudera.org:8080/#/c/13883/15/be/src/runtime/blocking-row-batch-queue.h@48 PS15, Line 48: The queue supports limiting the capacity in terms of bytes enqueued and number of : /// batches to be enqueued. > Stale ? Done -- 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: 16 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: Wed, 31 Jul 2019 00:22:20 +0000 Gerrit-HasComments: Yes
