impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4862: make resource profile consistent with backend behaviour
Date Mon, 10 Jul 2017 21:58:05 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4862: make resource profile consistent with backend behaviour
......................................................................


Patch Set 16:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/7223/16/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

PS16, Line 394: total
> total claims
Done


http://gerrit.cloudera.org:8080/#/c/7223/16/fe/src/main/java/org/apache/impala/planner/ParallelPlanner.java
File fe/src/main/java/org/apache/impala/planner/ParallelPlanner.java:

Line 89:       // We already recursed on the join build fragment in createBuildPlan().
> is this a bug fix independent of this patch? this statement isn't obviously
Yes, the problem was that createBuildPlan() was not correctly updating the fragment tree,
and this function was implicitly depending on that bug.

Before these fixes we got obviously incorrect output in the planner tests - the resource consumption
of a query was greater than the sum of all operators because operators were double-counted.


http://gerrit.cloudera.org:8080/#/c/7223/16/fe/src/main/java/org/apache/impala/planner/PlanFragment.java
File fe/src/main/java/org/apache/impala/planner/PlanFragment.java:

PS16, Line 210: Compute the resource profile of the fragment
> I think this should say something about being host wide (i.e. for all insta
Done


Line 224:       // finish.
> where does that happen?
JoinNode.computeTreeResourceProfiles(). Added a reference in the comment.


Line 385:     builder.append("Per-Host Resources: ");
> did you and Alex already decide that this should be output regardless of ex
Maybe I misunderstood the question, but we only output the fragment header at EXTENDED explain
level and above anyway - this patch doesn't change that.

Yeah Per-Host was meant to differentiate that.


http://gerrit.cloudera.org:8080/#/c/7223/16/fe/src/main/java/org/apache/impala/planner/PlanNode.java
File fe/src/main/java/org/apache/impala/planner/PlanNode.java:

PS16, Line 617: resources
> peak resources?
Done


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

PS16, Line 342: the given plans
> obvious*
I augmented the comment to explicitly mention fragment instances.


PS16, Line 354: Sum of per-host minimum reservations over all plan nodes and sinks.
> that's still the ambiguous way of describing it. How about rewording that t
I thought it was useful to describe how it was computed. Reworded the comment a bit to be
more concise and explain what it is before how it is computed.


PS16, Line 366: so we are
              :       // forced to
> forced to what?
Oops didn't finish writing this comment (or left it in an editor buffer).


http://gerrit.cloudera.org:8080/#/c/7223/16/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
File testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test:

PS16, Line 141: mem-estimate=15.68KB mem-reservation=136.00MB
> does it make sense for the estimate ever less than the reservation? i guess
In the current patch, no, since the reservations aren't really enforced.

With the switch-over to the buffer pool that would make sense. Filed IMPALA-5641 to keep track
of it - it will require updating a bunch of planner tests so probably best to do it as a separate
patch.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I492cf5052bb27e4e335395e2a8f8a3b07248ec9d
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message