Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201707062219.v66MJi7b005412@ip-10-146-233-104.ec2.internal>
Date: Thu, 6 Jul 2017 22:19:44 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Tim Armstrong <tarmstrong@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Alex Behm <alex.behm@cloudera.com>
Reply-To: dhecht@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4862=3A_make_resource_profile_consistent_with_backend_behaviour=0A?=
In-Reply-To: <gerrit.1497891244000.I492cf5052bb27e4e335395e2a8f8a3b07248ec9d@gerrit.cloudera.org>
References: <gerrit.1497891244000.I492cf5052bb27e4e335395e2a8f8a3b07248ec9d@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Thu, 06 Jul 2017 22:19:50 -0000

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