Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201611302305.uAUN5w0M001358@ip-10-146-233-104.ec2.internal>
Date: Wed, 30 Nov 2016 23:05:58 +0000
From: "Marcel Kornacker (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Henry Robinson <henry@cloudera.com>, Matthew Jacobs <mj@cloudera.com>,
        Alex Behm <alex.behm@cloudera.com>, Lars Volker <lv@cloudera.com>,
        Sailesh Mukil <sailesh@cloudera.com>
Reply-To: marcel@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4014=3A_Introduce_query-wide_execution_state=2E=0A?=
In-Reply-To: <gerrit.1473900487000.I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b@gerrit.cloudera.org>
References: <gerrit.1473900487000.I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b@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: Wed, 30 Nov 2016 23:06:06 -0000

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4014: Introduce query-wide execution state.
......................................................................


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4418/5/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:

PS5, Line 72: qs->refcnt_.Add(1);
> Another example: an ExecPlanFragment RPC can arrive late after some instanc
what you're describing is a complete corner case. i don't see a need to optimize for that.


Line 141: 
> With the current state of things, after all fragment instances finish execu
i do not want to introduce another counter to keep track of fragment instances, the whole point of this is to keep the qs around until all references to it are gone (and not just those needed for instance execution - i'd prefer to have the final status report rpc be sent when the qs gets gc'ed).

those late-arriving rpcs you're describing sound like another corner case, and i don't see how they would end up consuming a lot of resources. i would be worried about them if they increased the map lock contention by a sizable amount, say 30 or 50 or 100%, but i just don't see how that's possible.

aside from that, it's very easy to reduce the lock contention by an arbitrary amount (by partitioning the key space and having a separate map plus lock per partition). i just don't think we need that quite yet.


http://gerrit.cloudera.org:8080/#/c/4418/5/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

PS5, Line 351: TUniqueId no_instance_id_;
> Sorry I meant that it doesn't seem to be initialized. Where is it initializ
the default c'tor initializes it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes