Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201705240011.v4O0BKA7017435@ip-10-146-233-104.ec2.internal>
Date: Wed, 24 May 2017 00:11:20 +0000
From: "Henry Robinson (Code Review)" <gerrit@cloudera.org>
To: Marcel Kornacker <marcel@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Reply-To: henry@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4890/5143=3A_Coordinator_race_involving_TearDown=28=29=0A?=
In-Reply-To: <gerrit.1494952760000.I457a6424a0255c137336c4bc01a6e7ed830d18c7@gerrit.cloudera.org>
References: <gerrit.1494952760000.I457a6424a0255c137336c4bc01a6e7ed830d18c7@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.7
archived-at: Wed, 24 May 2017 00:11:26 -0000

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4890/5143: Coordinator race involving TearDown()
......................................................................


Patch Set 1:

(1 comment)

Patch looks pretty reasonable, will finish shortly.

http://gerrit.cloudera.org:8080/#/c/6897/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 895
> i think the waitforbackendcompletion call should be removed altogether, but
I believe that PlanRootSink() may set eos in GetNext() at the same time as it returns the final rows, if the sender calls Send() immediately followed by FlushFinal() before the consumer gets woken up in GetNext(). It looks to me like the callers expect that this might be possible. But since the PRS puts those results into a QueryResultSet owned by the ClientRequestState, I think it's safe to tear down the PRS once it sets eos.

As regards post-query finalization - what about computing the final profiles based on the last report sent by each fragment? Do we need to wait before calling Coordinator::ComputeQuerySummary()? I think we should aim to be rid of WaitForBackendCompletion() here, but maybe it still needs to exist in some form on a tear-down path which is not on the user's critical path.


-- 
To view, visit http://gerrit.cloudera.org:8080/6897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I457a6424a0255c137336c4bc01a6e7ed830d18c7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes