impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4862: make resource profile consistent with backend behaviour
Date Thu, 06 Jul 2017 23:13:45 GMT
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 <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