impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6121: remove I/O mgr request context cache
Date Thu, 09 Nov 2017 18:58:59 GMT
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8408 )

Change subject: IMPALA-6121: remove I/O mgr request context cache
......................................................................


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.h
File be/src/runtime/disk-io-mgr-reader-context.h:

http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.h@25
PS7, Line 25: /// Internal per request-context state. This object maintains a lot of state
that is
so no one should include this file except for disk io mgr, is that right? i.e. this class
is opaque to everyone else?

I think that's less obvious now that it's not in an -internal.h header.


http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.cc
File be/src/runtime/disk-io-mgr-reader-context.cc:

http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr-reader-context.cc@108
PS7, Line 108:   state_ = Inactive;
why is it that Cancel() has to do so much more work than this, when this supposedly does a
Cancel in addition to marking it inactive?  Is this really a different kind of cancel?


http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h@654
PS7, Line 654:   std::unique_ptr<DiskIoRequestContext> RegisterContext(MemTracker* reader_mem_tracker);
I'm not really sure what it means to "register" a context. Should this just be Create()? 
(And I guess we have these next three methods to keep the context opaque outside of disk-io-mgr,
is that right?)


http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h@662
PS7, Line 662:   void UnregisterContext(DiskIoRequestContext* context);
> I don't feel strongly. 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.  I'm fine with leaving it around.

But, what does it mean to "unregister" the context? The whole register/unregister terminology
seems confusing. Does the lifecycle of these things mirror the lifecycle that we're moving
toward and does it make sense to use consistent terminology?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91414eceaa4938fccd74686fe6bebede6ef36108
Gerrit-Change-Number: 8408
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Nov 2017 18:58:59 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message