impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Date Tue, 27 Sep 2016 23:35:48 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Chen Huang <paulhuang34@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message