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

2016-09-28 Thread Internal Jenkins (Code Review)
Internal Jenkins has posted comments on this change.

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


Patch Set 10: Verified+1

-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


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

2016-09-28 Thread Internal Jenkins (Code Review)
Internal Jenkins has submitted this change and it was merged.

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


IMPALA-3201: in-memory buffer pool implementation

This patch implements basic in-memory buffer management, with
reservations managed by ReservationTrackers.

Locks are fine-grained so that the buffer pool can scale to many
concurrent queries.

Includes basic tests for buffer pool setup, allocation and reservations.

Change-Id: I4bda61c31cc02d26bc83c3d458c835b0984b86a0
Reviewed-on: http://gerrit.cloudera.org:8080/4070
Reviewed-by: Tim Armstrong 
Tested-by: Internal Jenkins
---
M be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-allocator.cc
A be/src/bufferpool/buffer-allocator.h
M be/src/bufferpool/buffer-pool-test.cc
M be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
M be/src/util/internal-queue.h
M common/thrift/generate_error_codes.py
8 files changed, 933 insertions(+), 69 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I4bda61c31cc02d26bc83c3d458c835b0984b86a0
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong 


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

2016-09-28 Thread Internal Jenkins (Code Review)
Internal Jenkins has posted comments on this change.

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


Patch Set 10: Verified-1

Build failed: 
http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/240/

-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


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

2016-09-28 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

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


Patch Set 10: Code-Review+2

Rebase, carry +2

-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


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

2016-09-28 Thread Tim Armstrong (Code Review)
Hello Internal Jenkins, Dan Hecht,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4070

to look at the new patch set (#10).

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

IMPALA-3201: in-memory buffer pool implementation

This patch implements basic in-memory buffer management, with
reservations managed by ReservationTrackers.

Locks are fine-grained so that the buffer pool can scale to many
concurrent queries.

Includes basic tests for buffer pool setup, allocation and reservations.

Change-Id: I4bda61c31cc02d26bc83c3d458c835b0984b86a0
---
M be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-allocator.cc
A be/src/bufferpool/buffer-allocator.h
M be/src/bufferpool/buffer-pool-test.cc
M be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
M be/src/util/internal-queue.h
M common/thrift/generate_error_codes.py
8 files changed, 933 insertions(+), 69 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/4070/10
-- 
To view, visit http://gerrit.cloudera.org:8080/4070
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4bda61c31cc02d26bc83c3d458c835b0984b86a0
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong 


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

2016-09-28 Thread Internal Jenkins (Code Review)
Internal Jenkins has posted comments on this change.

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


Patch Set 9: Verified+1

-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


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

2016-09-27 Thread Dan Hecht (Code Review)
Dan Hecht has posted comments on this change.

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


Patch Set 8: Code-Review+2

(2 comments)

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

PS8, Line 25:  
are?


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

PS8, Line 280: internal::AtomicInt 
AtomicInt64


-- 
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


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

2016-09-27 Thread Tim Armstrong (Code Review)
Tim Armstrong has uploaded a new patch set (#8).

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

IMPALA-3201: in-memory buffer pool implementation

This patch implements basic in-memory buffer management, with
reservations managed by ReservationTrackers.

Locks are fine-grained so that the buffer pool can scale to many
concurrent queries.

Includes basic tests for buffer pool setup, allocation and reservations.

Change-Id: I4bda61c31cc02d26bc83c3d458c835b0984b86a0
---
M be/src/bufferpool/CMakeLists.txt
A be/src/bufferpool/buffer-allocator.cc
A be/src/bufferpool/buffer-allocator.h
M be/src/bufferpool/buffer-pool-test.cc
M be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
M be/src/util/internal-queue.h
M common/thrift/generate_error_codes.py
8 files changed, 933 insertions(+), 69 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/4070/8
-- 
To view, visit http://gerrit.cloudera.org:8080/4070
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4bda61c31cc02d26bc83c3d458c835b0984b86a0
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 


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

2016-09-27 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

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


Patch Set 4:

(2 comments)

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

PS4, Line 31: BufferAllocator
> SystemAllocator?
Ignore, this was a note-to-self.


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

Line 246: while (handle->is_pinned()) UnpinLocked(client, handle);
> I think we need to handle this differently.
Ignore, this was a note-to-self.


-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


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

2016-09-27 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

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


Patch Set 7:

(11 comments)

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

PS4, Line 31: Implement free 
SystemAllocator?


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

Line 246: 
I think we need to handle this differently.

Using ExtractBuffer() if it's pinned will avoid us shuffling it into the free 
lists.


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).
Done


PS7, Line 276: page->buffer.is_open()
> why not page->pinned?  Would be equivalent but seems more direct in intent.
An unpinned page may still have a buffer associated with it if it hasn't been 
evicted. In that case we should not allocate a new buffer.

Currently we never evict the page so checking page->pinned would always hit a 
DCHECK in AllocateBufferInternal() where we try to overwrite the open buffer 
hanlde.


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
Done


Line 382:   int pin_count() const { return pin_count_; }
> why do we need to expose this?
I added it for a unit test, but I didn't seen any reason to hide it - the 
client code should know the pin count implicitly anyway.


Line 414:   void DecrementPinCount();
> if we get rid of pin_count_ in the handle and only store that in the page (
Done


Line 421:   const Client* client_;
> let's consider not having this as it doesn't seem to add much value. we can
My concern was that if some code mistakenly provided the wrong client to a 
BufferPool API call, we could go a long way off into the weeds before the 
mistake became apparent - could be tricky to debug. Worse, it might actually 
succeed a lot of the time leaving a latent bug.


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


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


Line 432:   int pin_count_;
> why do we have this and the pinned_ boolean in the page?  i.e why not just 
Done


-- 
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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


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

2016-09-27 Thread Dan Hecht (Code Review)
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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-HasComments: Yes


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

2016-09-23 Thread Dan Hecht (Code Review)
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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-HasComments: Yes