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 <tarmstr...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to