Tim Armstrong has posted comments on this change. Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue. ......................................................................
Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4350/3/be/src/util/blocking-queue.h File be/src/util/blocking-queue.h: Line 99: bool BlockingPut(const T& val) { I think there's a pretty bad bug with how the signalling works. Consider a queue with a limit of N. If the queue gets into a state where get_list has N elements and put_list has 0 elements with all of the producer threads waiting on it, then the caller to BlockingGet() will drain get_list before signalling put_cv, even though it should wake up a producer thread once there is space in the queue. Worse, it will only wake up one producer thread each time BlockingGet() is called, which means that one only producer thread can run at a time. -- 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: 3 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
