impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3611: track unused Disk IO buffer memory
Date Thu, 09 Jun 2016 20:49:24 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-3611: track unused Disk IO buffer memory
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/3246/4/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:

PS4, Line 210: Clear(
Will Reset() be a better name ? This seems to match Reset() below.


PS4, Line 219:   mem_tracker_ = NULL;
nit: Can this be at line 213 so we initialize the fields in the same order as they are declared
?

Why is it okay to not initialize scan_range_offset_ ? I guess it's always overwritten when
the buffer is consumed, right ?


PS4, Line 648:       // Not a cached buffer. Return the io buffer.
             :       ReturnFreeBuffer(buffer_desc);
nit: should fit in one line.


Line 781:     if (!FLAGS_disable_mem_pools && free_buffers_[idx].size() < FLAGS_max_free_io_buffers)
{
Long line.

Please also see comments below about free_buffer_mem_tracker.


PS4, Line 856: bool any_io_mgr_limit_exceeded = free_buffer_mem_tracker_->AnyLimitExceeded();
Will it be more obvious if we move this line into ReturnFreeBuffer() instead ? That should
help reduce chances of failure due to exceeding memory limits, right ?

if (!FLAGS_disable_mem_pools &&
    free_buffers_[idx].size() < FLAGS_max_free_io_buffers &&
    free_buffer_mem_tracker_->TryConsume(buffer_size)) {
 . ...
} else {
  ...
}


-- 
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: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message