impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
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:

(19 comments)

http://gerrit.cloudera.org:8080/#/c/5847/3//COMMIT_MSG
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
usable.


http://gerrit.cloudera.org:8080/#/c/5847/3/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java
File fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java:

Line 249:     perHostMemCost_ = 0L;
does this make any difference in java?


http://gerrit.cloudera.org:8080/#/c/5847/3/fe/src/main/java/org/apache/impala/planner/PipelinedPlanNodeSet.java
File fe/src/main/java/org/apache/impala/planner/PipelinedPlanNodeSet.java:

Line 69:    * Computes the estimated per-host memory and CPU requirements of this plan node
set.
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_


http://gerrit.cloudera.org:8080/#/c/5847/3/fe/src/main/java/org/apache/impala/planner/Planner.java
File fe/src/main/java/org/apache/impala/planner/Planner.java:

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.
mysterious


http://gerrit.cloudera.org:8080/#/c/5847/3/fe/src/test/java/org/apache/impala/planner/PlannerTest.java
File fe/src/test/java/org/apache/impala/planner/PlannerTest.java:

Line 369:     options.setNum_scanner_threads(1); // Required so that output doesn't vary by
machine
please also add tests for mt execution


http://gerrit.cloudera.org:8080/#/c/5847/3/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java:

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


Line 420:             actualOutput);
odd indentation, please fix throughout


http://gerrit.cloudera.org:8080/#/c/5847/3/testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
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?


http://gerrit.cloudera.org:8080/#/c/5847/3/testdata/workloads/functional-planner/queries/PlannerTest/resource-estimates.test
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 http://gerrit.cloudera.org:8080/5847
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e358182bcf2bc5fe5c73883eb97878735b12d37
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message