impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3902: Scheduler improvements for running multiple fragment instances on a single backend
Date Sun, 21 Aug 2016 01:08:06 GMT
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

Mime
View raw message