Dan Hecht has posted comments on this change. Change subject: IMPALA-4862: make resource profile consistent with backend behaviour ......................................................................
Patch Set 13: (6 comments) The backend changes look good. I'll make another pass through the frontend, but let's first finalize the thrift change to make sure we're on the same page. http://gerrit.cloudera.org:8080/#/c/7223/13/be/src/exec/exec-node.h File be/src/exec/exec-node.h: PS13, Line 88: t nit: double space http://gerrit.cloudera.org:8080/#/c/7223/13/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: PS13, Line 395: does not overlap. shouldn't that be: do overlap? PS13, Line 398: per-host minimum buffer reservations that makes it sound like it's directly related to the thing above. maybe say instead: Sum of the per-plan-node minimum buffer reservations over all operators in the query plan. The fact that it's per-host is kind of implied then (since we're talking about nodes in the plan), and seems secondary to the fact that it's over the plan anyway (given what it's for). PS13, Line 400: per_host_min_reservation_su that name makes it sound like it's a sum of the previous field. Would sum_per_node_min_reservation or total_per_node_min_reservation make sense? http://gerrit.cloudera.org:8080/#/c/7223/13/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: Line 662: // then closed before Open() of this node returns. but that's not true if InSubplan, right? http://gerrit.cloudera.org:8080/#/c/7223/13/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: PS13, Line 354: // Sum of per-host minimum reservations over all plan nodes and sinks. Used to manage : // a pool of initial reservations: once this amount of initial reservation has been : // claimed, no more initial reservations will be claimed. : long perHostMinimumReservationSum = 0; let's rename this to be consistent with whatever we converge on for the thrift naming and thrift comment -- 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: 13 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
