Sahil Takiar 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 It actually has to check if the query has been cancelled or if the sink has been closed (because the sink can be closed before is_cancelled() is set to true). I changed IsQueueClosedOrEmpty() back to IsQueueEmpty(), so now all the loop have to check if the query has been cancelled or if the sink has been closed before checking if the queue is empty. Since that check has to be repeated in multiple places, I moved it into a method called IsCancelledOrClosed. I'm not sure if thats much better than IsQueueClosedOrEmpty - the reason I'm adding the helper method is to try and simply the already complex condition in the while loops. -- 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 <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Comment-Date: Sat, 14 Sep 2019 00:53:57 +0000 Gerrit-HasComments: Yes
