Dan Hecht has posted comments on this change.

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


Patch Set 7:

(8 comments)

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

PS7, Line 66:  > 0
delete (though this may become pin_count anyway).


PS7, Line 276: page->buffer.is_open()
why not page->pinned?  Would be equivalent but seems more direct in intent.


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

PS7, Line 262:  
bytes


Line 382:   int pin_count() const { return pin_count_; }
why do we need to expose this?


Line 414:   void DecrementPinCount();
if we get rid of pin_count_ in the handle and only store that in the page (see 
comment below), then these wouldn't be needed and it would be one less 
abstraction level to think about.


Line 421:   const Client* client_;
let's consider not having this as it doesn't seem to add much value. we can 
always add it later if necessary.


Line 429:   const BufferHandle* buffer_handle_;
this would also go away.


Line 432:   int pin_count_;
why do we have this and the pinned_ boolean in the page?  i.e why not just have 
a single count in the page and no state in the handle?


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