Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201711091858.vA9IwxeZ007342@ip-10-146-233-104.ec2.internal>
Date: Thu, 9 Nov 2017 18:58:59 +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: Michael Ho <kwho@cloudera.com>, Tianyi Wang <twang@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-6121=3A_remove_I/O_mgr_request_context_cache=0A?=
In-Reply-To: <gerrit.1509147433000.I91414eceaa4938fccd74686fe6bebede6ef36108@gerrit.cloudera.org>
References: <gerrit.1509147433000.I91414eceaa4938fccd74686fe6bebede6ef36108@gerrit.cloudera.org>
Reply-To: dhecht@cloudera.com, impala-cr@cloudera.com, twang@cloudera.com,
        marcelk@gmail.com, tarmstrong@cloudera.com, kwho@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="i5hCzUsbV9M="; charset=UTF-8
archived-at: Thu, 09 Nov 2017 19:06:14 -0000

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

Dan Hecht has posted comments on this change=2E ( http://gerrit=2Ecloudera=
=2Eorg:8080/8408 )

Change subject: IMPALA-6121: remove I/O mgr request con=
text cache
=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


=
Patch Set 7:

(4 comments)

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8408/7/=
be/src/runtime/disk-io-mgr-reader-context=2Eh
File be/src/runtime/disk-io-m=
gr-reader-context=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8408/7/be/s=
rc/runtime/disk-io-mgr-reader-context=2Eh@25
PS7, Line 25: /// Internal per=
 request-context state=2E This object maintains a lot of state that is
so n=
o one should include this file except for disk io mgr, is that right? i=2Ee=
=2E this class is opaque to everyone else?

I think that's less obvious now=
 that it's not in an -internal=2Eh header=2E


http://gerrit=2Ecloudera=2Eo=
rg:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context=2Ecc
File be/s=
rc/runtime/disk-io-mgr-reader-context=2Ecc:

http://gerrit=2Ecloudera=2Eorg=
:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context=2Ecc@108
PS7, Li=
ne 108:   state_ =3D Inactive;
why is it that Cancel() has to do so much mo=
re work than this, when this supposedly does a Cancel in addition to markin=
g it inactive?  Is this really a different kind of cancel?


http://gerrit=
=2Ecloudera=2Eorg:8080/#/c/8408/7/be/src/runtime/disk-io-mgr=2Eh
File be/sr=
c/runtime/disk-io-mgr=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8408/7/=
be/src/runtime/disk-io-mgr=2Eh@654
PS7, Line 654:   std::unique_ptr<DiskIoR=
equestContext> RegisterContext(MemTracker* reader_mem_tracker);
I'm not rea=
lly sure what it means to "register" a context=2E Should this just be Creat=
e()?  (And I guess we have these next three methods to keep the context opa=
que outside of disk-io-mgr, is that right?)


http://gerrit=2Ecloudera=2Eor=
g:8080/#/c/8408/7/be/src/runtime/disk-io-mgr=2Eh@662
PS7, Line 662:   void =
UnregisterContext(DiskIoRequestContext* context);
> I don't feel strongly=
=2E I think your argument has merit but destroying the d
Yeah, except that =
the statement "context cannot be used after this call" is what contradicts =
the control structure lifetime being tied to QueryState=2E  I'm fine with l=
eaving it around=2E

But, what does it mean to "unregister" the context? Th=
e whole register/unregister terminology seems confusing=2E Does the lifecyc=
le of these things mirror the lifecycle that we're moving toward and does i=
t make sense to use consistent terminology?


-- 
To view, visit http://ge=
rrit=2Ecloudera=2Eorg:8080/8408
To unsubscribe, visit http://gerrit=2Ecloud=
era=2Eorg:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
G=
errit-MessageType: comment
Gerrit-Change-Id: I91414eceaa4938fccd74686fe6beb=
ede6ef36108
Gerrit-Change-Number: 8408
Gerrit-PatchSet: 7
Gerrit-Owner: Tim=
 Armstrong <tarmstrong@cloudera=2Ecom>
Gerrit-Reviewer: Dan Hecht <dhecht@c=
loudera=2Ecom>
Gerrit-Reviewer: Michael Ho <kwho@cloudera=2Ecom>
Gerrit-Rev=
iewer: Tianyi Wang <twang@cloudera=2Ecom>
Gerrit-Reviewer: Tim Armstrong <t=
armstrong@cloudera=2Ecom>
Gerrit-Comment-Date: Thu, 09 Nov 2017 18:58:59 +0=
000
Gerrit-HasComments: Yes

--i5hCzUsbV9M=--