impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3202: implement spill-to-disk in new buffer pool
Date Wed, 25 Jan 2017 22:57:30 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3202: implement spill-to-disk in new buffer pool

Patch Set 13:

File be/src/runtime/bufferpool/buffer-pool-internal.h:

Line 67: /// the client before closing IMPALA-3202.
> yeah let's figure this out after this patch. i'm not sure such a tight coup
I was thinking that we don't really want client code to call many of the ReservationTracker
methods. E.g. calling AllocateFrom() or ReleaseTo() doesn't make sense. And to decrease the
reservation we would also need to write out some pages.

Line 241:     return !buffer.is_open();
> this is kind of a weird interface given that the answer may change immediat
It can't actually change asynchronously out of the evicted state.

But yeah, I agree the abstraction wasn't helping much, so I just inlined at the handful of

Line 264:   ConditionVariable write_complete_cv_;
> doesn't this pretty much duplicate the write_handle's condvar? would it be 
It gets messy with the callbacks and the lifecycle of the WriteHandle.

The specific problem is that the callback from WriteHandle::WriteComplete() can cause the
destruction of the WriteHandle(), so there would be no condition variable left to signal.

I actually realised that there was a race there if a thread sees that the write is finished
and destroys the WriteHandle before NotifyAll() is called, so I fixed that too.

PS13, Line 267: or evicted
> but how do you know if it's been evicted without calling IsEvicted() which 
Yeah you can infer it.

Or if you called IsEvicted() earlier, since nothing will asynchronously move it out of the
evicted state.

PS13, Line 267: unpinned
> shouldn't this say "pinned" rather than "unpinned"?
File be/src/runtime/bufferpool/

Line 464:   }
> but just to be clear, i'm not suggesting we should address it in this patch
Yeah I agree. I think we'll actually contend much worse on TCMalloc's global heap lock currently,
so I'm looking at solving both problems at the same time.
File be/src/runtime/bufferpool/

PS13, Line 407: CHECK
> did you mean DCHECK?
File be/src/runtime/bufferpool/buffer-pool.h:

Line 224:   /// any other operations for 'client'.
> why is it important to allow that?
We'll need once we switch over DiskIoMgr to use these buffers, since we then will attach the
buffers to RowBatches in the scanner, then send them to a different thread through the queue.
That thread may free or take ownership of the buffer.

Every other resource that can be attached to a RowBatch has this property, and the BufferPool
implementation naturally supports it, so it made sense to me just to document it.

Line 232:   /// concurrently with any other operations for 'src_client'.
> and here?
See above.
File be/src/runtime/

Line 1222:   unique_lock<mutex> writer_lock(writer->lock_);
> why is this removed?
We can hit this with the variable-length write support. I just found this while doing some

The DCHECK was spurious anyway, since the limitation is not documented in the interface and
there is no reason the implementation can't handle writing longer buffers.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c6119a4557da853fefa02e17c49d8be0e9fbe01
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message