impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Date Wed, 28 Sep 2016 02:27:23 GMT
Michael Ho has posted comments on this change.

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


Patch Set 5:

(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: ck_) / 
> should this be total_put_wait_time_ (i.e. the last "put" field)?  If we wan
Sure, we can use 'pad_' as point of division here. It seems like a bit of an overkill to have
to come up with extra structs which make the code less readable.


PS7, Line 74:  signal
> NotifyAll() is not used here to avoid ...
Done


Line 74:         // avoid the race in which the writer may be signalled between when it checks
> ... when it calls Wait() in BlockingPut().
Done


PS7, Line 100: h HDFS scan no
> is this explanation specific to HDFS scan node? We also use this queue in e
Actually, I find this sentence a bit misleading after re-reading the comment again. Removed.


PS7, Line 171: changed once t
> why is this okay for the callers outside this class? we still have the spli
Yes, this can be split into SizeLocked() which requires "put_lock_" be held and externally
facing Size().

In general, there is no guarantee the queue size will not change after the function Size()
returns so there is really no "true" 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
Done


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