Michael Ho has posted comments on this change. Change subject: IMPALA-3201: headers and reservation logic for new buffer pool ......................................................................
Patch Set 23: (8 comments) http://gerrit.cloudera.org:8080/#/c/2569/23/be/src/bufferpool/buffer-allocator.cc File be/src/bufferpool/buffer-allocator.cc: PS23, Line 19: BufferAllocator::BufferAllocator(int64_t min_buffer_len): min_buffer_len_(min_buffer_len) {} nit: long line. PS23, Line 28: int64_t len Is this needed or are you intentionally leaving this for extension to mmap() later ? http://gerrit.cloudera.org:8080/#/c/2569/23/be/src/bufferpool/buffer-pool.cc File be/src/bufferpool/buffer-pool.cc: PS23, Line 40: Decrement Increment PS23, Line 55: --pin_count DCHECK_GT(pin_count, 0); before this line ?! PS23, Line 354: "Buffer bytes limit $0 of buffer " nit: Long line http://gerrit.cloudera.org:8080/#/c/2569/23/be/src/bufferpool/buffer-pool.h File be/src/bufferpool/buffer-pool.h: Line 66: /// stays mapped to the same buffer. Do pinning and unpinning always happen via a page handle ? If so, can you please briefly document that ? PS23, Line 101: BufferPool::CloseHandle() Does closing a page handle implicitly unpin the page it references ? PS23, Line 107: duplicating it Reading this header file, it's not entirely clear how handle's duplication interacts with a pinned handle. In particular, it's not entirely clear whose reservations will be updated when unpinning happens on a duplicated handle. Does duplicating a pinned handle consume the reservation of the recipient ? I suppose one can find the answer by reading the code by it may help readers to briefly explain the contract here. -- To view, visit http://gerrit.cloudera.org:8080/2569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I35cc89e863efb4cc506657bfdaaaf633a10bbab6 Gerrit-PatchSet: 23 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
