Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201607210508.u6L5850C023932@ip-10-146-233-104.ec2.internal>
Date: Thu, 21 Jul 2016 05:08:05 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Tim Armstrong <tarmstrong@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
CC: Michael Ho <kwho@cloudera.com>
Reply-To: dhecht@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-CR=5D=28cdh5-trunk=29_IMPALA-3611=3A_track_unused_Disk_IO_buffer_memory=0A?=
In-Reply-To: <gerrit.1464712861000.I8777cf76f04d34a46f53d53005412e0f1d63b5b7@gerrit.cloudera.org>
References: <gerrit.1464712861000.I8777cf76f04d34a46f53d53005412e0f1d63b5b7@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Thu, 21 Jul 2016 05:08:12 -0000

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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes