Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201711011734.vA1HYgkr015600@ip-10-146-233-104.ec2.internal>
Date: Wed, 1 Nov 2017 17:34:41 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: anujphadke <aphadke@cloudera.com>,
        Joe McDonnell <joemcdonnell@cloudera.com>,
        Dan Hecht <dhecht@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-1575=3A_part_2=3A_yield_admission_control_resources=0A?=
In-Reply-To: <gerrit.1508369448000.I80279eb2bda740d7f61420f52db3bfa42a6a51ac@gerrit.cloudera.org>
References: <gerrit.1508369448000.I80279eb2bda740d7f61420f52db3bfa42a6a51ac@gerrit.cloudera.org>
Reply-To: tarmstrong@cloudera.com, impala-cr@cloudera.com,
        aphadke@cloudera.com, marcelk@gmail.com, dhecht@cloudera.com,
        joemcdonnell@cloudera.com, reviews@impala.incubator.apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="V/I0QOHsRRU="; charset=UTF-8
archived-at: Wed, 01 Nov 2017 17:34:49 -0000

--V/I0QOHsRRU=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hello anujphadke, Joe McDonnell, Dan Hecht, 

I'd like you to reexamine a c=
hange=2E Please visit

    http://gerrit=2Ecloudera=2Eorg:8080/8323

to loo=
k at the new patch set (#5)=2E

Change subject: IMPALA-1575: part 2: yield =
admission control resources
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E

IMPALA-1575: part 2: yield admission control resources

Th=
is change releases admission control resources more eagerly,
once the query=
 has finished actively executing=2E Some resources
(tracked and untracked) =
are still consumed by the client request
as long as it remains open, e=2Eg=
=2E memory for control structures
and the result cache=2E However, these re=
sources are relatively
small and should not block admission of new queries=
=2E

The same as in part 1, query execution is considered to be finished
un=
der any of the following conditions:
1=2E The query encounters an error and=
 fails
2=2E The query is cancelled due to the idle query timeout
3=2E The q=
uery reaches eos (or the DML completes)
4=2E The client cancels the query w=
ithout closing the query

Admission control resources are released in two w=
ays:
1=2E by calling AdmissionController::ReleaseQuery() on the coordinator=

   promptly after query execution finishes, instead of waiting for
   Unre=
gisterQuery()=2E This means that the query and its memory is
   no longer c=
onsidered "admitted"=2E
2=2E by changing the behaviour of MemTracker::GetPo=
olMemReserved() so
   that it is aware of when a query has finished executi=
ng and does not
   consider its entire memory limit to be "reserved"=2E

Th=
e preconditions for releasing an admitted query are subtle because the
quer=
ies are being admitted to a distributed system, not just the
coordinator=2E=
  The comment for ReleaseAdmissionControlResources()
documents the precondi=
tions and rationale=2E Note that the preconditions
are not weaker than the =
preconditions of calling UnregisterQuery()
before this patch=2E

Testing:
T=
estAdmissionController is extended to end queries in four ways:
cancellatio=
n by client, idle timeout, the last row being fetched,
and the client closi=
ng the query=2E The test uses a mix of all four=2E
After the query ends, al=
l clients wait for the test to complete
before closing the query or closing=
 the connection=2E This ensures
that the admission control decisions are ba=
sed entirely on the
query end behavior=2E This test works for both query ad=
mission control
and mem_limit admission control and can detect both kinds o=
f admission
control resources ("admitted" and "reserved") not being release=
d
promptly=2E

This is based on an earlier patch by Joe McDonnell=2E

Chang=
e-Id: I80279eb2bda740d7f61420f52db3bfa42a6a51ac
---
M be/src/runtime/coordi=
nator=2Ecc
M be/src/runtime/coordinator=2Eh
M be/src/runtime/mem-tracker=2E=
cc
M be/src/runtime/mem-tracker=2Eh
M be/src/runtime/query-state=2Ecc
M be/=
src/scheduling/admission-controller=2Ecc
M be/src/scheduling/admission-cont=
roller=2Eh
M be/src/service/client-request-state=2Ecc
M tests/custom_cluste=
r/test_admission_controller=2Epy
9 files changed, 192 insertions(+), 87 del=
etions(-)


  git pull ssh://gerrit=2Ecloudera=2Eorg:29418/Impala-ASF refs/=
changes/23/8323/5
-- 
To view, visit http://gerrit=2Ecloudera=2Eorg:8080/83=
23
To unsubscribe, visit http://gerrit=2Ecloudera=2Eorg:8080/settings

Gerr=
it-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchse=
t
Gerrit-Change-Id: I80279eb2bda740d7f61420f52db3bfa42a6a51ac
Gerrit-Change=
-Number: 8323
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <tarmstrong@cl=
oudera=2Ecom>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera=2Ecom>
Gerrit-Rev=
iewer: Joe McDonnell <joemcdonnell@cloudera=2Ecom>
Gerrit-Reviewer: Tim Arm=
strong <tarmstrong@cloudera=2Ecom>
Gerrit-Reviewer: anujphadke <aphadke@clo=
udera=2Ecom>

--V/I0QOHsRRU=--