Tim Armstrong 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 Added a comment explaining that. Some clients need to include it to make the unique_ptr destructor work (that destructor has a static assert that sizeof(DiskIoRequestContext) > 0). 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 su This function calls Cancel() on line 95. 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 I think it's an implementation detail the RegisterContext() doesn't register it in any data structures. UnregisterContext() *does* have to track down and remove references from I/O manager internal data structures. http://gerrit.cloudera.org:8080/#/c/8408/7/be/src/runtime/disk-io-mgr.h@662 PS7, Line 662: void UnregisterContext(DiskIoRequestContext* context); > Yeah, except that the statement "context cannot be used after this call" is "unregister" to me means removing all references to it from the I/O manager. I think the name conveys that the I/O manager may hold references to it. Reworked the comment a bit. I can imagine adding operations to DiskIoRequestContext that would be reasonable to call after UnregisterContext(), e.g. exposing the current state of the context. I'm fine with switching to passing in the unique_ptr, can do that in the next PS if it seems appropriate. -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 09 Nov 2017 23:53:36 +0000 Gerrit-HasComments: Yes
