impala-dev mailing list archives

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

Change subject: IMPALA-3201: buffer pool header only
......................................................................


Patch Set 5:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/3688/5/be/src/bufferpool/buffer-pool.h
File be/src/bufferpool/buffer-pool.h:

Line 85: /// it is able to fulfill the reservations when needed, e.g. by flushing unpinned
pages
> i think it would be good to create a condensed version of the specific guar
I deleted the middle paragraph here and cut down the other paragraphs, just to focus on the
idea that reservations are necessary and sufficient for allocating memory. I think the method
comments should be the canonical reference for more specific guarantees.


Line 90: /// The buffer pool does not directly expose its internal data structures to clients,
> opportunity for more brevity: no need to explain general software engineeri
Done


Line 109: /// The invariants are as follows:
> update this section
Done


Line 113: /// * Pin() can be called on a pinned handle, incrementing the handle's pin count.
> it sounds like you're mixing up 'pinned' and 'open'. the distinction isn't 
Done


Line 124: ///   destroy the handle and free the page and its buffer, or use TransferBuffer()
to
> from the perspective of the caller, it's not clear why/how a page is involv
editing error, removed.


Line 138: /// * If the operator is done with the page, it can call DestroyBuffer() to destroy
the
> why DestroyBuffer?
Editing error - DestroyPage(). Fixed.


Line 203:   /// buffers or disk storage backing the page is freed. Idempotent.
> should this be idempotent (but unpinning an unpinned page isn't)?
The pins are referenced counted, so unpinning twice is different from unpinning once. We could
change it so that unpinning an unpinned page has no effect, but I think that would make it
easier to be sloppy.

Idempotency of DestroyPage() is mainly to simplify cleanup logic so it doesn't have to check
whether everything is open.


Line 223:   Status TransferBuffer(Client* src_client, BufferHandle* src, Client* dst_client,
> can't the handle stay the same? what is the effect on the src's and dst's r
Made the reservation implications of the ownership change explicit in the comment.

No reason it can't be transferred in-place. I did it this way since usually it makes sense
to move to a new handle when transferring ownership. E.g. attaching it to a row batch. This
would discourage patterns where the ownership of a a particular member variable or vector
of handles was ambiguous.


Line 277:   int64_t len() const;
> is this legal for unpinned pages?
Yeah, the length of the page's data still makes sense if the page is unpinned.


-- 
To view, visit http://gerrit.cloudera.org:8080/3688
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id771dea2eb4c1aa13c30d59e8b184a7d1bca8d34
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message