Dan Hecht has posted comments on this change. Change subject: IMPALA-3611: track unused Disk IO buffer memory ......................................................................
Patch Set 9: (9 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_); it's a bit confusing that the tracking happens both in BufferDescriptor methods (here) and also directly in DiskIoMgr methods. maybe there's no great way to factor the code to keep the accounting all in once class though. PS9, Line 368: Untracked this is a little misleading given that they are tracked by this tracker. Why not just make a mem tracker for buffered block manager, and then nothing has to change in this code when the buffered block manager is deleted? is there something that made that tough? PS9, Line 650: Not a cached buffer. Return the io buffer. not your comment, but this comment is pretty useless, it just restates what the C++ code says. it'd be better to explain why cached buffers are not returned. Line 752: delete[] buffer; would be nice to delete first then Release(). http://gerrit.cloudera.org:8080/#/c/3246/9/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: PS9, Line 242: caller should check the memory limit do callers actually check the mem limit? or are they assuming that the mem limit is okay because the src and dst mem trackers are within the same hierarchy and dst itself doesn't have a limit? PS9, Line 271: Non-NULL for non-cached double negative is hard to parse. maybe say "Can be NULL only for cached" PS9, Line 774: associated what does this mean? does this routine consume memory from 'mem_tracker', or does this mean something else? Line 792: /// max_buffer_size_. would be good to comment on how mem tracking is affected. Line 802: /// Returns the buffer in 'desc' (cannot be NULL), and sets desc->buffer_ to NULL. likewise. from the comments alone it's not clear what the difference between ReturnBuffer() and ReturnFreeBuffer() are. -- 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: 9 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
