Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id CE7DE200CC5 for ; Tue, 11 Jul 2017 09:00:06 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id CB2511644F6; Tue, 11 Jul 2017 07:00:06 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id 1E1051644A9 for ; Tue, 11 Jul 2017 09:00:05 +0200 (CEST) Received: (qmail 253 invoked by uid 500); 11 Jul 2017 07:00:05 -0000 Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list reviews@impala.incubator.apache.org Received: (qmail 234 invoked by uid 99); 11 Jul 2017 07:00:04 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd3-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 11 Jul 2017 07:00:04 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd3-us-west.apache.org (ASF Mail Server at spamd3-us-west.apache.org) with ESMTP id 2E23E195847 for ; Tue, 11 Jul 2017 07:00:04 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 0.363 X-Spam-Level: X-Spam-Status: No, score=0.363 tagged_above=-999 required=6.31 tests=[RDNS_DYNAMIC=0.363, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=disabled Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id weiiWtHPUshV for ; Tue, 11 Jul 2017 07:00:02 +0000 (UTC) Received: from ip-10-146-233-104.ec2.internal (ec2-75-101-130-251.compute-1.amazonaws.com [75.101.130.251]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with ESMTPS id 78B1362EBE for ; Tue, 11 Jul 2017 06:48:38 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by ip-10-146-233-104.ec2.internal (8.14.4/8.14.4) with ESMTP id v6B6mb88009833; Tue, 11 Jul 2017 06:48:37 GMT Message-Id: <201707110648.v6B6mb88009833@ip-10-146-233-104.ec2.internal> Date: Tue, 11 Jul 2017 06:48:37 +0000 From: "Tim Armstrong (Code Review)" To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org CC: Dan Hecht Reply-To: tarmstrong@cloudera.com X-Gerrit-MessageType: comment Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4674=3A_Part_2=3A_port_backend_exec_to_BufferPool=0A?= X-Gerrit-Change-Id: I7fc7fe1c04e9dfb1a0c749fb56a5e0f2bf9c6c3e X-Gerrit-ChangeURL: X-Gerrit-Commit: 81261cd9e3f42f9a7f53816fadffd8fda2f462fd In-Reply-To: References: 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, 11 Jul 2017 07:00:07 -0000 Tim Armstrong has posted comments on this change. Change subject: IMPALA-4674: Part 2: port backend exec to BufferPool ...................................................................... Patch Set 23: (3 comments) http://gerrit.cloudera.org:8080/#/c/5801/23/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: Line 209: &buffer_pool_client_, resource_profile_.min_reservation); > What I mean is wouldn't it be incorrect to return min_reservation if the cl Oh yes, you're right. Added documentation for that requirement in the header. It would hit a DCHECK in ReservationTracker. http://gerrit.cloudera.org:8080/#/c/5801/25/be/src/exec/hash-table.h File be/src/exec/hash-table.h: PS25, Line 532: f th > double into Done http://gerrit.cloudera.org:8080/#/c/5801/23/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: PS23, Line 403: > I'm not sure why it's better in the case of cancelation, since the old code I guess it's better in that most of the resources of a large query will be released even if one fragment is slow to finish. It sounds potentially hairy if we do it the wrong way. I don't think we want the Coordinator to deal with the resources being released asynchronously by the executing fragments. It might worth though if we also refcounted the QueryState resources and released the coordinator's resource refcount in Coordinator::ReleaseResources(). -- To view, visit http://gerrit.cloudera.org:8080/5801 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7fc7fe1c04e9dfb1a0c749fb56a5e0f2bf9c6c3e Gerrit-PatchSet: 23 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes