Dan Hecht has posted comments on this change. Change subject: IMPALA-5113: fix dirty unpinned invariant ......................................................................
Patch Set 4: (8 comments) Nice cleanup. http://gerrit.cloudera.org:8080/#/c/6469/4/be/src/runtime/bufferpool/buffer-pool-internal.h File be/src/runtime/bufferpool/buffer-pool-internal.h: PS4, Line 219: or ? Line 222: /// locks should be held by the caller. this should talk specifically about reservations. PS4, Line 233: CleanPagesBeforeAllocationLocked the name seems a bit misleading since we have to use it in places other than buffer allocation. If we remove CleanPagesBeoreAllocation() (see comments in cc file), maybe just rename this to CleanPages() or similar. Line 237: /// accounting. No page or client locks should be held by the caller. let's mention specifically that it releases the buffer to the client's reservation. PS4, Line 336: Only used for debugging. remove this now that the byte count is presumably used for non-debugging. http://gerrit.cloudera.org:8080/#/c/6469/4/be/src/runtime/bufferpool/buffer-pool.cc File be/src/runtime/bufferpool/buffer-pool.cc: Line 131: client->impl_->reservation()->AllocateFrom(len); why does this happen after AllocateBufferInternal()? In the case of AllocateBuffer(), the reservation consumption happens first, right? Also, the documentation for AllocateBufferInternal() says it assumes the reservation has already been updated. Also, it's a little unfortunate that we have to expose both CleanPagesBeforeAllocation() and PrepareToAllocateBuffer() from the client (I guess since this path we don't want to account it as an allocated buffer). I wonder if it'd be better to revert this change, and make AddNewPinnedPage() decrement buffers_allocated_bytes_ since its job is to effectively finalizing the page mapping in the client accounting. Line 505: // Clean pages before updating the accounting. that's obvious from the code. explain the "why" instead. http://gerrit.cloudera.org:8080/#/c/6469/4/be/src/runtime/bufferpool/buffer-pool.h File be/src/runtime/bufferpool/buffer-pool.h: Line 251: class PageList; is this only for tests? if so, let's comment that. -- To view, visit http://gerrit.cloudera.org:8080/6469 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I07e08acb6cf6839bfccbd09258c093b1c8252b25 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
