impala-reviews mailing list archives

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

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


Patch Set 16:

(11 comments)

I only did a quick look through the .test file changes. LMK if there's something specific
there I should verify.

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

PS16, Line 394: total
total claims


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 true given the
comment on collectJoins(). do we have sufficient test coverage here?


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 instances not just for
one instance of this plan fragment).


Line 224:       // finish.
where does that happen?


Line 385:     builder.append("Per-Host Resources: ");
did you and Alex already decide that this should be output regardless of explain level? Notes
we only print at TExplainLevel.EXTENDED, right?

It's a bit subtle that this includes all instances, whereas at the node level it will be for
a single instance, but I guess the "Per-Host" is meant to differentiate that?


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

Line 117:   protected ResourceProfile nodeResourceProfile_ = ResourceProfile.invalid();
it seems a little weird that the this doesn't really have a single meaning. It kind of depends
on the type of plan node and then is interpreted appropriately by computeTreeResourceProfiles().
But you don't have to revisit this now.


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?


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
I don't think it's opposite that this incorporates DOP.


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 to be more similar
to the thrift comment. Also say "initial" rather than "minimum"?


PS16, Line 366: so we are
              :       // forced to
forced to what?


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 it's saying we
don't expect a full buffer to be used, so useful to not clip it?


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