Dan Hecht has posted comments on this change. Change subject: IMPALA-3611: track unused Disk IO buffer memory ......................................................................
Patch Set 11: Code-Review+2 (4 comments) http://gerrit.cloudera.org:8080/#/c/3246/9/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: Line 246: mem_tracker_->Release(buffer_len_); > I feel like this actually makes sense, since the tracking in DiskIoMgr meth Okay, that's fine. PS9, Line 368: Untracked > I originally called it "Other Disk IO Buffers" but changed it as a a result Okay, fine to leave as is as long as this goes away with buffer pool. http://gerrit.cloudera.org:8080/#/c/3246/9/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: Line 802: > Renamed to FreeBufferMemory() and updated the comment. Thanks for the rename. http://gerrit.cloudera.org:8080/#/c/3246/10/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: Line 779: MemTracker* mem_tracker, ScanRange* range, char* buffer, int64_t buffer_size); let's group GetBufferDesc and GetFreeBuffer decls together, and group ReturnBufferDesc and ReturnBuffer decls, since they have analogous relationships. And it'd be nice to name them consistently to make this relationship more apparent. i.e. GetBuffer()/GetBufferDesc() or GetFreeBuffer()/GetFreeBufferDesc(), to be analogous to ReturnBuffer()/ReturnBufferDesc(). But okay to ignore this rename if you don't prefer. -- To view, visit http://gerrit.cloudera.org:8080/3246 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8777cf76f04d34a46f53d53005412e0f1d63b5b7 Gerrit-PatchSet: 11 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
