Tim Armstrong 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& ? Done 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? Probably not. The old message wasn't actionable either by a user (beyond reporting the bug). Reworded to make it clear that it's an internal error. Line 708: DCHECK_LE(buffer_size, max_buffer_size_); > how do we ensure this is true? is it the check in DiskIoMgr::Read() that wi DiskIoMgr::ReadRange() checks if there's a client buffer before calling TryAllocateNextBufferForRange(). 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: Yes that's better... 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)? Tried to clarify. Also removed the last sentence since I think it was confusing and not really useful to the reader. PS1, Line 392: bool > maybe make all of these members 'const' since they are "options" that don't Done PS1, Line 422: BufferOpts > const BufferOpts& ? Done Line 506: int64_t client_buffer_len_; > maybe move this right before cached_buffer_ since it's related in the sense Switched it to a tagged union. We don't really have much precedent in the codebase for this pattern so I invented some conventions that made sense to me. -- 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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
