Dan Hecht has posted comments on this change.

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


Patch Set 7:

(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: put_cv_
should this be total_put_wait_time_ (i.e. the last "put" field)?  If we want to 
do this kind of check, it would be more robust to group put related fields in 
one struct, get stuff in another struct, then have this class contain a get and 
put field of these struct types, and write the assert that verifies those 
structs don't overlap cache lines.


PS7, Line 74: Calling
NotifyAll() is not used here to avoid ...


Line 74:         // the queue size and when it calls Wait(). Calling 
NotifyAll() here may trigger
... when it calls Wait() in BlockingPut().


PS7, Line 100: HDFS scan node
is this explanation specific to HDFS scan node? We also use this queue in e.g. 
Kudu scan node and also ThreadPool.


PS7, Line 171: put_list_.size
why is this okay for the callers outside this class? we still have the split 
read problem.  also, the comment is a bit hand-wavy given that this could 
return a value that is never the true size of the queue. e.g. while swapping, 
might a caller see twice one of the deque sizes making it look like the queue 
is over capacity?  or, we could see a value too small, and that would violate 
the assumption being made in ImpalaServer::MembershipCallback() when it checks 
the work 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


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