impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3201: buffer pool header only
Date Mon, 08 Aug 2016 23:18:34 GMT
Marcel Kornacker 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 guarantees of the buffer
pool, without comments about what clients might be doing or what might be going on under the
covers. possibly as an introductory section to this class comment, if we feel we also need
the expanded version.


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 engineering practices in
class comments.


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


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 particularly clear.


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 involved.


Line 138: /// * If the operator is done with the page, it can call DestroyBuffer() to destroy
the
why DestroyBuffer?


Line 203:   /// buffers or disk storage backing the page is freed. Idempotent.
should this be idempotent (but unpinning an unpinned page isn't)?


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 reservations?


Line 277:   int64_t len() const;
is this legal for unpinned pages?


-- 
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