Michael Ho has posted comments on this change. Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue ......................................................................
Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/4350/7/be/src/util/blocking-queue.h File be/src/util/blocking-queue.h: PS7, Line 55: ck_) / > should this be total_put_wait_time_ (i.e. the last "put" field)? If we wan Sure, we can use 'pad_' as point of division here. It seems like a bit of an overkill to have to come up with extra structs which make the code less readable. PS7, Line 74: signal > NotifyAll() is not used here to avoid ... Done Line 74: // avoid the race in which the writer may be signalled between when it checks > ... when it calls Wait() in BlockingPut(). Done PS7, Line 100: h HDFS scan no > is this explanation specific to HDFS scan node? We also use this queue in e Actually, I find this sentence a bit misleading after re-reading the comment again. Removed. PS7, Line 171: changed once t > why is this okay for the callers outside this class? we still have the spli Yes, this can be split into SizeLocked() which requires "put_lock_" be held and externally facing Size(). In general, there is no guarantee the queue size will not change after the function Size() returns so there is really no "true" queue size. http://gerrit.cloudera.org:8080/#/c/4350/7/be/src/util/condition-variable.h File be/src/util/condition-variable.h: PS7, Line 29: > let's clarify: boost thread interruption Done -- To view, visit http://gerrit.cloudera.org:8080/4350 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Chen Huang <paulhuan...@utexas.edu> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
