Dan Hecht has posted comments on this change. Change subject: IMPALA-3202: DiskIoMgr improvements for new buffer pool ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/4631/1/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: PS1, Line 193: BufferOpts const& ? http://gerrit.cloudera.org:8080/#/c/4631/1/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: PS1, Line 642: without the client providing a " : "buffer" is this going to make sense to a user? Line 708: DCHECK_LE(buffer_size, max_buffer_size_); how do we ensure this is true? is it the check in DiskIoMgr::Read() that will always enforce? http://gerrit.cloudera.org:8080/#/c/4631/1/be/src/runtime/disk-io-mgr.h File be/src/runtime/disk-io-mgr.h: PS1, Line 147: these buffers are returned to the caller wrapped : /// in BufferDescriptors. took a few seconds to parse and understand. how about: ... these buffers are wrapped in BufferDescriptors and return to the caller. Line 150: /// another read. In error cases, the IoMgr will recycle the buffers more promptly but is this relevant for all cases, or only cases (a) and (b)? PS1, Line 392: bool maybe make all of these members 'const' since they are "options" that don't change, right? PS1, Line 422: BufferOpts const BufferOpts& ? Line 506: int64_t client_buffer_len_; maybe move this right before cached_buffer_ since it's related in the sense that it's another possible source of the buffer. Alternatively, consider even making a union to make it clear that they are mutually exclusive, and the union tag indicates the buffer "mode" (Managed, HdfsCached, Client - or something like that). Though it could be that doesn't work out cleanly given that HdfsCached can fallback to Managed mode. -- To view, visit http://gerrit.cloudera.org:8080/4631 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I913fbb8ae6c242f1585992eb2a622509344dccec Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho Gerrit-HasComments: Yes
