[Impala-ASF-CR] IMPALA-3201: in-memory buffer pool implementation

["Dan Hecht (Code Review)" <ge...@cloudera.org>]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3201: in-memory buffer pool implementation
......................................................................


Patch Set 7:

(1 comment)

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

PS7, Line 423: cached locally to avoid acquiring the page lock
             :   /// unnecessarily.
I'm not sure this make sense. one way to look at it is if it's safe to cache this value, then it means that the underlying page_'s len/buffer can't be changing (or else the cached value is wrong), and therefore it must be safe to access those fields directly, right? 

another way to look at is is that the whole point of pinning is to ensure that the page to buffer mapping cannot change, and thus pinned pages' fields should be accessible without a lock.  any pages can only be pinned by a single client which implies a single thread.

besides, what other thread should be able to modify (or even access) a pinned page?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4bda61c31cc02d26bc83c3d458c835b0984b86a0
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-HasComments: Yes