Tim Armstrong has posted comments on this change. Change subject: IMPALA-4862: make resource profile consistent with backend behaviour ......................................................................
Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/7223/7/fe/src/main/java/org/apache/impala/planner/ParallelPlanner.java File fe/src/main/java/org/apache/impala/planner/ParallelPlanner.java: Line 82: private void recomputeResourceProfiles() { > This seems weird to me and I believe we can potentially get rid of this if Done. Reworked this so that the traversal logic is all in computeResourceReqs() Line 196: This didn't connect up the fragments correctly. http://gerrit.cloudera.org:8080/#/c/7223/7/fe/src/main/java/org/apache/impala/planner/PlanVisitor.java File fe/src/main/java/org/apache/impala/planner/PlanVisitor.java: Line 6: public interface PlanVisitor { > We already have a Visitor and a corresponding accept() function. Can we con Removed this in favour of doing an explicit traversal in computeResourceReq() http://gerrit.cloudera.org:8080/#/c/7223/7/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: Line 352: private static class ResourceAggregator implements PlanVisitor { > Do we really need this separate visitor that only sums over things we have Was able to just do an explicit traversal in computeResourceReq(). Line 392: planRoots.get(0).walkPlan(aggregator); > The new approach makes sense to me - I like it. PlanTreeResources vs. ResourceProfile: * Updated the ResourceProfile comment * Renamed PlanTreeResources to PhaseResourceProfiles, since it's really just a wrapper around ResourceProfiles for two different phases of execution. Visitor and recursion: * I was able to rework this so that the whole traversal is done iteratively in this function using collect()/getNodesPostOrder(). There's no need for a visitor then. Entry point: this is now not called by finalize(). Renamed finalize() to finalizeExchanges() since that is more precisely what it does. http://gerrit.cloudera.org:8080/#/c/7223/7/fe/src/main/java/org/apache/impala/planner/ResourceProfile.java File fe/src/main/java/org/apache/impala/planner/ResourceProfile.java: Line 23: * The resources that will be consumed by a set of plan nodes. > Does this really represent the resources of a single PlanNode instance or P Updated the comment. This abstraction is agnostic about what collection of things it is a profile for - it really is just a container for a set of peak values. -- 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: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
