impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3201: in-memory buffer pool implementation
Date Sat, 24 Sep 2016 00:53:50 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message