Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8023 )
Change subject: IMPALA-4856: Port data stream service to KRPC ...................................................................... Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@211 PS8, Line 211: deferred_rpcs_.empty() && batch_queue_.empty() > why not write this condition as: Actually, I think the loop exiting condition is not quite right, which led to this confusin conditional. The loop exiting condition for "we're done" should check that there are no more senders, and that there's nothing left to drain from the deferred_rpcs_ queue and that there's no pending insert into batch_queue_. So, the third wait loop condition should be something like: (num_remaining_senders > 0 || !deferred_rpcs_.empty() || num_pending_enqueue_ > 0) and then this if-stmt conditional can just be: if (batch_queue_.empty()) and then the DCHECK can be the negation of that third wait loop conditional: // Wait loop is exited with an empty batch_queue_ only when there will be no more batches. DCHECK(num_remaining_senders == 0 && deferred_rpcs_.empty() && num_pending_enqueue_ == 0); And then you can get rid of the outer loop. That outer loop should be removed since it's effectively a busy wait (and I think we could get into a busy wait state in the previous patchsets). -- To view, visit http://gerrit.cloudera.org:8080/8023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic0b8c1e50678da66ab1547d16530f88b323ed8c1 Gerrit-Change-Number: 8023 Gerrit-PatchSet: 8 Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Tue, 07 Nov 2017 15:58:21 +0000 Gerrit-HasComments: Yes
