Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201608210108.u7L186qD012935@ip-10-146-233-104.ec2.internal>
Date: Sun, 21 Aug 2016 01:08:06 +0000
From: "Alex Behm (Code Review)" <gerrit@cloudera.org>
To: Marcel Kornacker <marcel@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
Reply-To: alex.behm@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3902=3A_Scheduler_improvements_for_running_multiple_fragment_instances_on_a_single_backend=0A?=
In-Reply-To: <gerrit.1471565970000.I240445199e22f009f4e72fdb8754eb8d77e3d680@gerrit.cloudera.org>
References: <gerrit.1471565970000.I240445199e22f009f4e72fdb8754eb8d77e3d680@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: Sun, 21 Aug 2016 01:08:18 -0000

Alex Behm has posted comments on this change.

Change subject: IMPALA-3902: Scheduler improvements for running multiple fragment instances on a single backend
......................................................................


Patch Set 3:

(5 comments)

Had a pass over the FE. Looks good overall, just had some ideas for factoring out common code.

http://gerrit.cloudera.org:8080/#/c/4054/3/fe/src/main/java/com/cloudera/impala/common/TreeNode.java
File fe/src/main/java/com/cloudera/impala/common/TreeNode.java:

Line 63:   protected <C extends TreeNode<NodeType>>void getNodesPreOrderAux(ArrayList<C> result) {
space before void


http://gerrit.cloudera.org:8080/#/c/4054/3/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:

Line 964:       planner.computeResourceReqs(fragments, true, queryExecRequest);
This function assumes that the entire query is captured by the given fragments, so it won't work for the MT case.

It will set an 'arbitrary' resource estimate in queryExecRequest based on the planRoot that was processed last.

I'd suggest setting dummy values in mtCreateExecRequest() and adding a TODO to fix it.


Line 981:    * Create a populated TQueryExecRequest, corresponding to the supplied TQueryCtx,
fix comment: input is a planner


Line 998:     TExplainLevel explainLevel = TExplainLevel.EXTENDED;
The explain level setting is somewhat subtle, maybe factor out that part.

For example, move the code into planner.getExplainString() and don't pass in a level.

or add a Planner.getEffectiveExplainLevel() that contains this logic


Line 1042:     for (int idx = 0; idx < fragments.size(); ++idx) {
factor our the common code between createExecRequest() and createPlanExecInfo()

It looks like the differences are mostly due to the different type of 'result', but that can probably be factored out by passing collection references to the new helper function, and then setting those in the caller's result. For example,

void helper(List<Integer> dest_fragment_idx, Map<Integer TScanRangeLocations>> per_node_scan_ranges);

then at the caller:

List<Integer> dest_fragment_idx;
Map<Integer, TScanRangeLocations> per_node_scan_ranges;
helper(dest_fragment_idx, per_node_scan_ranges);
result.setDest_fragment_idx(dest_fragment_idx);
result.setPer_node_scan_ranges(per_node_scan_ranges);


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I240445199e22f009f4e72fdb8754eb8d77e3d680
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-HasComments: Yes