Michael Ho has posted comments on this change. Change subject: IMPALA-3611: track unused Disk IO buffer memory ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/3246/3/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: PS3, Line 688: BufferedBlockMgr does not expect read buffers to be tracked : // against its MemTracker Why is it ? PS3, Line 691: reader->mem_tracker_ != NULL They are essentially untracked memory., right ? How about having a dummy tracker for this until we fix IMPALA-3200 ? PS3, Line 834: buffer_mem_tracker_->AnyLimitExceeded() Why do we want to cancel the query just because we have too many free buffers ? Is it essentially checking for process memory limit here ? If so, the old code seems more explicit about it. It seems unfortunate that we still have some query maintenance like code in this module. http://gerrit.cloudera.org:8080/#/c/3246/3/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: PS3, Line 240: /// Updates this buffer buffer to be owned by the new tracker. Consumption is : /// released from the current tracker and added to the new one. If either tracker : /// is NULL, the Release() or Consume() call for that tracker is skipped. I.e. : /// if 'tracker' is NULL, the memory is just released from 'mem_tracker_', and if : /// 'mem_tracker_' is NULL, only consumption of 'tracker' is increased. This seems a bit complicated. Given there are only two callers (i.e. ReturnFreeBuffer() and RowBatch::AddIoBuffer()), I wonder if it's clearer to do the explicit ownership transfer by calling MemTracker::Release()/Consume() directly at those call sites. I find reasoning about whether tracker / mem_tracker_ is NULL a bit too convoluted. -- 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: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
