Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201609152034.u8FKYaQT007357@ip-10-146-233-104.ec2.internal>
Date: Thu, 15 Sep 2016 20:34:36 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: Michael Ho <kwho@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Mostafa Mokhtar <mmokhtar@cloudera.com>,
        Chen Huang <paulhuang34@utexas.edu>,
        Alex Behm <alex.behm@cloudera.com>, Dan Hecht <dhecht@cloudera.com>
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4026=3A_Implement_double-buffering_for_BlockingQueue=2E=0A?=
In-Reply-To: <gerrit.1473445502000.Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54@gerrit.cloudera.org>
References: <gerrit.1473445502000.Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Thu, 15 Sep 2016 20:34:41 -0000

Tim Armstrong has posted comments on this change.

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


Patch Set 3:

(1 comment)

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

Line 99:   bool BlockingPut(const T& val) {
> Yes, I have thought about it before. I have tested various locations for no
We discussed this a little offline and I feel like I understand why option (3) works in practice now - when N producers are outpacing consumers, this switches to a mode where the steady state is a nearly empty queue with k producers sitting blocked and N - k producers working. Essentially there are some additional elements in the queue because the blocked producers also have some row batches ready to add to the queue immediately.

I'm ok moving forward with it so long as we document the expected behaviour. Perhaps it's adequate to document the expect behaviour with a faster consumer, matched producer/consumer and fast producers.

Have you benchmarked this with concurrent queries or workloads with many concurrent scans? One concern with the empty queue is that if the queue is empty and a producer isn't scheduled immediately, the consumer may end up waiting on the condition variable for every batch and potentially getting swapped it. We could perhaps work around that if the producer added its item to the queue *before* blocking, so that the the consumer can get the item without requiring a context switch.


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