Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201708151726.v7FHQoAV019883@ip-10-146-233-104.ec2.internal>
Date: Tue, 15 Aug 2017 17:26:50 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Matthew Jacobs <mj@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Lars Volker <lv@cloudera.com>, Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: dhecht@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4833=3A_Compute_precise_per-host_reservation_size=0A?=
In-Reply-To: <gerrit.1502319910000.Idbcd1e9b1be14edc4017b4907e83f9d56059fbac@gerrit.cloudera.org>
References: <gerrit.1502319910000.Idbcd1e9b1be14edc4017b4907e83f9d56059fbac@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: Tue, 15 Aug 2017 17:26:56 -0000

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4833: Compute precise per-host reservation size
......................................................................


Patch Set 8:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/7630/8/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:

PS8, Line 56: fragments
instances


Line 57:   // instance_params.
I think this comment lost some information compared to the equivalent comments in the old code.  From the comment, it's hard to know whether this is the minimum over all instances of the reservation, or the total of all the minimums. It would help to explain what this is conceptually.


PS8, Line 62: initial_reservation_total_bytes
I found that having the word "claim" in the old equivalent members made it easier to keep track of what this was, since "total" can be so vague.


http://gerrit.cloudera.org:8080/#/c/7630/8/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

PS8, Line 399: in the case all fragments are scheduled on all hosts with
             :   // max DOP
given that this patch is addressing the overestimation, why do we compute this overestimate?


http://gerrit.cloudera.org:8080/#/c/7630/8/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

PS8, Line 483: fragments in fragment_ctxs
is it over fragments or instances?  if fragments and not instances, how does that work out?


PS8, Line 490: initial_reservation_total_bytes
same comment


-- 
To view, visit http://gerrit.cloudera.org:8080/7630
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idbcd1e9b1be14edc4017b4907e83f9d56059fbac
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes