[Impala-ASF-CR] IMPALA-3201: in-memory buffer pool implementation
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
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
Internal Jenkins has posted comments on this change. Change subject: IMPALA-3201: in-memory buffer pool implementation .. Patch Set 10: Build failed: http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/243/ -- 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
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
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
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
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
Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: in-memory buffer pool implementation .. Patch Set 9: Code-Review+2 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: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3201: in-memory buffer pool implementation
Tim Armstrong has posted comments on this change. Change subject: IMPALA-3201: in-memory buffer pool implementation .. Patch Set 8: (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? Done 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 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: 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
Hello 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 (#9). 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/9 -- 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: 9 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
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
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
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
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
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
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
