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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
