Tim Armstrong has posted comments on this change. Change subject: IMPALA-4862: make resource profile consistent with backend behaviour ......................................................................
Patch Set 13: (6 comments) 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 Done 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? It's if they never overlap. Maybe it helps to mention that this allows reusing of reservations. PS13, Line 398: per-host minimum buffer reservations > that makes it sound like it's directly related to the thing above. maybe sa Done PS13, Line 400: per_host_min_reservation_su > that name makes it sound like it's a sum of the previous field. Would Done 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? Yeah this was missing documentation that the subplan root calculates the resource requirements instead of calling this function. I wasn't able to easily add a Precondition check that this was true - that would require some additional plumbing - only UnionNode currently keeps track of whether it's in a subplan. 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 thr Done -- 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
