impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3748: Part 1: Clean up resource estimation in planner
Date Mon, 06 Feb 2017 17:17:28 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-3748: Part 1: Clean up resource estimation in planner

Patch Set 3:

Commit Message:

Line 28: * Many operators do not have estimates or have questionable heuristics.
are you planning on addressing this particular point in a follow-up? the estimates will now
matter if they are used for reservations, and really bad estimates will make the system less
File fe/src/main/java/org/apache/impala/planner/

Line 249:     perHostMemCost_ = 0L;
does this make any difference in java?
File fe/src/main/java/org/apache/impala/planner/

Line 69:    * Computes the estimated per-host memory and CPU requirements of this plan node
remove references to cpu requirements

Line 105:         perHostHdfsScanMem += node.getPerHostMemCost();
this is incorrect for mt execution, because it extrapolates the number of executing threads
from the number of cores. please fix this in hdfsscannode.

Line 116:         Math.min(perHostHbaseScanMem, HBaseScanNode.getPerHostMemUpperBound());
same here

Line 123:       sink.computeCosts();
this is also wrong, because it doesn't take sorting into account.

you need to make sure the estimates are at least in the ballpark, otherwise we start rejecting
queries left and right that used to run.

Line 197:     // TODO: can overestimate resource consumption of UnionNodes - the execution
of union
this needs to get fixed. see earlier comment.

Line 205:    * Estimate of the peak resources that will be consumed by a set of plan nodes.
move above where it's used

Line 209:     private final boolean valid_;
prefixing boolean variables with 'is-" is helpful

Line 214:       this.valid_ = valid;
remove this_
File fe/src/main/java/org/apache/impala/planner/

Line 344:    * Estimates the per-host memory and CPU requirements for the given plan fragments,
remove references to cpu requirements (here and elsewhere)

Line 375:     if (isTrivialCoordOnlyPlan(fragments)) {
check early?

Line 381:     // excludeUnpartitionedFragments is true.
File fe/src/test/java/org/apache/impala/planner/

Line 369:     options.setNum_scanner_threads(1); // Required so that output doesn't vary by
please also add tests for mt execution
File fe/src/test/java/org/apache/impala/planner/

Line 413:             ignoreExplainHeader, errorLog, actualOutput);
odd indentation

Line 420:             actualOutput);
odd indentation, please fix throughout
File testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test:

Line 46: |  hosts=3 per-host-mem=160B
we need to impose a meaningful minimum, such as the minimal mempool size

Line 52: |  hosts=3 per-host-mem=128.00MB
where did that come from?
File testdata/workloads/functional-planner/queries/PlannerTest/resource-estimates.test:

Line 120: |  hosts=3 per-host-mem=10.00MB
this should express the minimum required to operate at all, rather than "how much would i
like to get"

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e358182bcf2bc5fe5c73883eb97878735b12d37
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

View raw message