Tim Armstrong has posted comments on this change. Change subject: IMPALA-5113: fix dirty unpinned invariant ......................................................................
Patch Set 4: (8 comments) 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 > ? Done Line 222: /// locks should be held by the caller. > this should talk specifically about reservations. Done PS4, Line 233: CleanPagesBeforeAllocationLocked > the name seems a bit misleading since we have to use it in places other tha Done 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 rese Done PS4, Line 336: Only used for debugging. > remove this now that the byte count is presumably used for non-debugging. Done 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 Allocat Changed so that new pages are constructed by calling AllocateBuffer() to allocate a buffer, then passing that into a different function that creates the page. Line 505: // Clean pages before updating the accounting. > that's obvious from the code. explain the "why" instead. Done 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. It needs to be a type in the BufferPool namespace because it accesses Page, which is private to that namespace. -- 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
