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 Thu, 06 Jul 2017 22:19:44 GMT
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 <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