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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Chen Huang <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
