impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
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:

File be/src/exec/exec-node.h:

PS13, Line 88: t  
> nit: double space
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

PS13, Line 400: per_host_min_reservation_su
> that name makes it sound like it's a sum of the previous field.  Would
File fe/src/main/java/org/apache/impala/planner/

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.
File fe/src/main/java/org/apache/impala/planner/

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I492cf5052bb27e4e335395e2a8f8a3b07248ec9d
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message