impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-3201: buffer pool header only
Date Mon, 15 Aug 2016 21:44:26 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3201: buffer pool header only

Patch Set 7:

File be/src/bufferpool/buffer-pool.h:

PS7, Line 124: free up memory reservations
> this instance wasn't updated to be clearer on terminology:

Line 155:   ///     remainder will not be usable.
> alternatively, express size as multiple of min_buffer_len
My thought was that it was best to consistently denominate buffers in bytes rather than use
two different units. There's a trade-off here in that we have to document what happens in
this case, but I like the consistency.

Line 201:   /// closed and 'buffer_handle' will reference the buffer from 'page_handle'. This
> "will be closed" = "will be destroyed", i assume? we seem to use close and 
The handle is closed and the page is destroyed. I'll use the second here since it's clearer.

PS7, Line 217: frees
> also not updated:

Line 218:   /// reservation in 'src_client'. 'src' must be open and 'dst' must be closed before
> how do you get a closed BufferHandle, other than calling allocate and free 
The default constructor of BufferHandle creates a closed buffer handle: it's only opened by
calling a buffer pool method with this as an argument. So the code looks like:

BufferHandle dst;
TransferBuffer(src_client, &src, dst_client, &dst);

Line 218:   /// reservation in 'src_client'. 'src' must be open and 'dst' must be closed before
> we can decide to change this later, if the code that calls this ends up loo
Sounds good.

Line 279:   const BufferHandle* buffer_handle() const;
> presumably an error to call FreeBuffer on the returned handle directly?
Right, that's what the const modifier is enforcing.

Extended comment to explain it.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id771dea2eb4c1aa13c30d59e8b184a7d1bca8d34
Gerrit-PatchSet: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message