Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: headers and reservation logic for new buffer pool ......................................................................
Patch Set 23: (8 comments) Addressed the comments. I will now move to Impala-ASF and reconcile it with the header patch. 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. Done PS23, Line 28: int64_t len > Is this needed or are you intentionally leaving this for extension to mmap( Exactly, this is just a stub implementation now but I wanted to have the more general interface. 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 Done PS23, Line 55: --pin_count > DCHECK_GT(pin_count, 0); before this line ?! Done PS23, Line 354: "Buffer bytes limit $0 of buffer " > nit: Long line Done 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 p I think the "Page Handles" section more-or-less covers this - all page operations go through a page handle. Added a brief clarification here that clients don't interact with pages directly. PS23, Line 101: BufferPool::CloseHandle() > Does closing a page handle implicitly unpin the page it references ? Answered in the next section of this comment and also in the CloseHandle() method comment: "A page is destroyed once the last open handle is closed via CloseHandle()." There's another corner-case where maybe you have two handles - one pinned and another unpinned, and you close the pinned one. In that case the underlying page becomes implicitly unpinned. This is ok since you couldn't access the page's buffer via the unpinned handle anyway. PS23, Line 107: duplicating it > Reading this header file, it's not entirely clear how handle's duplication I think it's implied by the DuplicateHandle() comment stating that the new handle starts with pin count 0. I added an additional clarification. The idea is to separate the duplication of handles from operations on reservations. -- 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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
