Tim Armstrong has posted comments on this change. Change subject: IMPALA-3202: implement spill-to-disk in new buffer pool ......................................................................
Patch Set 13: (10 comments) http://gerrit.cloudera.org:8080/#/c/5584/13/be/src/runtime/bufferpool/buffer-pool-internal.h 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 callsites. 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"? Done http://gerrit.cloudera.org:8080/#/c/5584/12/be/src/runtime/bufferpool/buffer-pool.cc File be/src/runtime/bufferpool/buffer-pool.cc: 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. http://gerrit.cloudera.org:8080/#/c/5584/13/be/src/runtime/bufferpool/buffer-pool.cc File be/src/runtime/bufferpool/buffer-pool.cc: PS13, Line 407: CHECK > did you mean DCHECK? Done http://gerrit.cloudera.org:8080/#/c/5584/13/be/src/runtime/bufferpool/buffer-pool.h 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. http://gerrit.cloudera.org:8080/#/c/5584/12/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: 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 experimenting. 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 http://gerrit.cloudera.org:8080/5584 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8c6119a4557da853fefa02e17c49d8be0e9fbe01 Gerrit-PatchSet: 13 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
