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