Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14214 )
Change subject: IMPALA-8924, IMPALA-8934: Result spooling failpoint tests, fix DCHECKs ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/14214/1/be/src/exec/buffered-plan-root-sink.cc File be/src/exec/buffered-plan-root-sink.cc: http://gerrit.cloudera.org:8080/#/c/14214/1/be/src/exec/buffered-plan-root-sink.cc@175 PS1, Line 175: while (IsQueueClosedOrEmpty() && sender_state_ == SenderState::ROWS_PENDING : && !state->is_cancelled() && !timed_out) { As discussed offline, the contract seems simpler if we make it an invariant that IsQueueEmpty() cannot be called after the plan root sink has been cancelled. IMHO, it seems a bit weird to still loop around if "the queue is closed" part is true. It seems to be making assumption that state->is_cancelled() is called afterwards. IMHO, the new interface IsQueueClosedOrEmpty() seems a tad error prone as it returns true for two possible states and these two states may potentially lead to drastically different actions (i.e. keep waiting vs breaking out of the loop). -- To view, visit http://gerrit.cloudera.org:8080/14214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib96f797bc8a5ba8baf9fb28abd1f645345bbe932 Gerrit-Change-Number: 14214 Gerrit-PatchSet: 1 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-Comment-Date: Fri, 13 Sep 2019 21:42:33 +0000 Gerrit-HasComments: Yes
