Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue
......................................................................


Patch Set 4:

(8 comments)

I'm ok with this option if it generally provides better perf and avoids 
pathological behaviour. Generally looks good, just comment and style comments.

http://gerrit.cloudera.org:8080/#/c/4350/4/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:

Line 36: /// if the queue is empty or full, respectively.
I think we should clarify when the unblocking happens. In BlockingGet() it is 
unblocked as soon as an item is added, but in BlockingPut() it is not 
guaranteed (since we're not calling put_cv_.NotifyOne() with the lock held).


Line 95:     put_cv_.NotifyOne();
Can you comment that this notification is racy: producers may not be notified 
if the queue is partially empty.


Line 182:   /// Maximum number of elements in 'get_list_' + 'put_list_'.
clarify as "Maximum total number" - if a reader wasn't careful they might thing 
it meant the maximum per list.


PS4, Line 190: > >
nit: we're generally using >> now that it works in c++11


http://gerrit.cloudera.org:8080/#/c/4350/4/be/src/util/condition-variable.h
File be/src/util/condition-variable.h:

Line 29: /// This has lower overhead than boost's implementation as it
Nit: lines are wrapped very short here


PS4, Line 31: CACHE_ALIGNED
Nit: here and in the other places: does it work to put this on the line before 
'class'? That seems a little more readable to me.


Line 43:   bool inline TimedWait(boost::unique_lock<boost::mutex>& lock,
What does the return value mean?


Line 50:   void inline NotifyOne() { pthread_cond_signal(&cv_); }
Nit: here and above, we usually put 'inline' before the return type.


-- 
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: 4
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

Reply via email to