Mailing-List: contact reviews-help@impala.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201711202324.vAKNOqB4026067@ip-10-146-233-104.ec2.internal>
Date: Mon, 20 Nov 2017 23:24:52 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: Tianyi Wang <twang@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-4835=3A_Part_1=3A_simplify_I/O_mgr_mem_mgmt_and_cancellation=0A?=
In-Reply-To: <gerrit.1509391479000.If5cb42437d11c13bc4a55c3ab426b66777332bd1@gerrit.cloudera.org>
References: <gerrit.1509391479000.If5cb42437d11c13bc4a55c3ab426b66777332bd1@gerrit.cloudera.org>
Reply-To: tarmstrong@cloudera.com, impala-cr@cloudera.com, twang@cloudera.com,
        marcelk@gmail.com, dhecht@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="fVITCZn8Xrw="; charset=UTF-8
archived-at: Mon, 20 Nov 2017 23:24:58 -0000

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

Hello Tianyi Wang, Dan Hecht, 

I'd like you to reexamine a change=2E Pleas=
e visit

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

to look at the new p=
atch set (#12)=2E

Change subject: IMPALA-4835: Part 1: simplify I/O mgr me=
m mgmt and cancellation
=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-4835: Part 1: simplify I/O mgr mem mgmt and cancellation
=

In preparation for switching the I/O mgr to the buffer pool, this
removes =
and cleans up a lot of code so that the switchover patch starts
from a clea=
ner slate=2E

* Remove the free buffer cache (which will be replaced by buf=
fer pool's
  own caching)=2E
* Make memory limit exceeded error checking sy=
nchronous (in anticipation
  of having to propagate buffer pool errors sync=
hronously)=2E
* Simplify error propagation - remove the (ineffectual) code =
that
  enqueued BufferDescriptors containing error statuses=2E
* Document l=
ocking scheme better in a few places, make it part of the
  function signat=
ure when it seemed reasonable=2E
* Move ReturnBuffer() to ScanRange, becaus=
e it is intrinsically
  connected with the lifecycle of a scan range=2E
* S=
eparate external ReturnBuffer() and internal CleanUpBuffer()
  interfaces -=
 previously callers of ReturnBuffer() were fudging
  the num_buffers_in_rea=
der accounting to make the external interface work=2E
* Eliminate redundant=
 state in ScanRange: 'eosr_returned_' and
  'is_cancelled_'=2E
* Clarify th=
e logic around calling Close() for the last
  BufferDescriptor=2E
  -> Ther=
e appeared to be an implicit assumption that buffers would be
     freed in=
 the order they were returned from the scan range, so that
     the "eos" b=
uffer was returned last=2E Instead just count the number
     of outstandin=
g buffers to detect the last one=2E
  -> Touching the is_cancelled_ field w=
ithout holding a lock was hard to
     reason about - violated locking rule=
s and it was unclear that it
     was race-free=2E
* Remove DiskIoMgr::Read=
() to simplify interface=2E It is trivial to
  inline at the callsites=2E

=
This will probably regress performance somewhat because of the cache
remova=
l, so my plan is to merge it around the same time as switching
the I/O mgr =
to allocate from the buffer pool=2E I'm keeping the patches
separate to mak=
e reviewing easier=2E

Testing:
* Ran exhaustive tests
* Ran the disk-io-mg=
r-stress-test overnight

Change-Id: If5cb42437d11c13bc4a55c3ab426b66777332b=
d1
---
M be/src/exec/hdfs-parquet-scanner=2Ecc
M be/src/exec/hdfs-scan-node=
-base=2Ecc
M be/src/exec/hdfs-scan-node=2Ecc
M be/src/exec/scanner-context=
=2Ecc
M be/src/runtime/exec-env=2Ecc
M be/src/runtime/io/disk-io-mgr-stress=
=2Ecc
M be/src/runtime/io/disk-io-mgr-test=2Ecc
M be/src/runtime/io/disk-io=
-mgr=2Ecc
M be/src/runtime/io/disk-io-mgr=2Eh
M be/src/runtime/io/request-c=
ontext=2Ecc
M be/src/runtime/io/request-context=2Eh
M be/src/runtime/io/req=
uest-ranges=2Eh
M be/src/runtime/io/scan-range=2Ecc
M be/src/runtime/mem-tr=
acker=2Eh
M be/src/runtime/test-env=2Ecc
M be/src/runtime/tmp-file-mgr-test=
=2Ecc
M be/src/runtime/tmp-file-mgr=2Ecc
M be/src/util/impalad-metrics=2Ecc=

M be/src/util/impalad-metrics=2Eh
19 files changed, 575 insertions(+), 904=
 deletions(-)


  git pull ssh://gerrit=2Ecloudera=2Eorg:29418/Impala-ASF r=
efs/changes/14/8414/12
-- 
To view, visit http://gerrit=2Ecloudera=2Eorg:80=
80/8414
To unsubscribe, visit http://gerrit=2Ecloudera=2Eorg:8080/settings
=

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpa=
tchset
Gerrit-Change-Id: If5cb42437d11c13bc4a55c3ab426b66777332bd1
Gerrit-C=
hange-Number: 8414
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong <tarmstr=
ong@cloudera=2Ecom>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera=2Ecom>
Gerr=
it-Reviewer: Tianyi Wang <twang@cloudera=2Ecom>
Gerrit-Reviewer: Tim Armstr=
ong <tarmstrong@cloudera=2Ecom>

--fVITCZn8Xrw=--