Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201701200126.v0K1QSBn010757@ip-10-146-233-104.ec2.internal>
Date: Fri, 20 Jan 2017 01:26:28 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Dan Hecht <dhecht@cloudera.com>
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3202=3A_implement_spill-to-disk_in_new_buffer_pool=0A?=
In-Reply-To: <gerrit.1482956696000.I8c6119a4557da853fefa02e17c49d8be0e9fbe01@gerrit.cloudera.org>
References: <gerrit.1482956696000.I8c6119a4557da853fefa02e17c49d8be0e9fbe01@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Fri, 20 Jan 2017 01:26:44 -0000

Tim Armstrong has posted comments on this change.

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


Patch Set 9:

(33 comments)

http://gerrit.cloudera.org:8080/#/c/5584/9/be/src/runtime/bufferpool/buffer-pool-internal.h
File be/src/runtime/bufferpool/buffer-pool-internal.h:

PS9, Line 31: earlier
> earlier than what? not sure what this means exactly.
Reworded to be hopefully cleared.


PS9, Line 48: evicted
> rather than define circularly, how about: after the clean page's buffer has
Done


PS9, Line 88: Does not update any
            :   /// reservations or the page's pin count.
> given the new naming, I think we can do without this sentence.
Done


PS9, Line 93:   /// Restore an unpinned page to the pinned state. Does not update any reservations or
            :   /// the page's pin count.
> I think this could use some rewording to be just in terms of client state m
Done


PS9, Line 99:  without pinned bytes plus dirty unpinned bytes exceeding the
            :   /// client's reservatio
> is the reasoning for this described anywhere? it would be good to explain t
I added a section to the implementation notes.


PS9, Line 114: WriteDirtyPages
> how about WriteDirtyPagesAsync()?
Done


Line 116:   /// Wait for the in-flight write for 'page'.
> to complete
Done


Line 132:   void CheckConsistency() {
> Maybe DCheckConsistency() to make to clear this is debug-only.
Done


PS9, Line 148: //
> ///
Done


PS9, Line 150: RepinEvictedPage
> maybe rename this one too since it's only doing part of the pinning work. m
Named it MoveEvictedToPinned() for consistency (it's really a special case of MoveToPinned()).


PS9, Line 153: //
> ///
Done


PS9, Line 177: the write
> what write? does this mean "the writes" or "a write"?
It isn't specific to a particular write. I expanded a bit about how it should be propagated.


Line 187:   InternalList<Page> pinned_pages_;
> why do we need this list? when a page is pinned, the buffer mapping can't b
Yeah it's just for debugging. I added a comment to reflect that, but we could just remove it also.


Line 195:   InternalList<Page> in_flight_write_pages_;
> this list also doesn't seem necessary. is it used for anything or just debu
We need to check whether it's in this state in various places. We could also add an 'in_flight' flag to the page to track the state, but it seemed cleaner to use this list to indicate the state - otherwise we're indicating some states by their presence in a list and other states by a flag on the page.


Line 246:   /// destruction of a page that was accessed via a data structure that is not PageHandle.
> this sentences seems to contradict the one earlier in this file about locks
Tried to clarify.


PS9, Line 249: Always open if pinned. Closed only if page
             :   /// is evicted.
> Should it now just say "Closed iff page is evicted. Open otherwise."?
Done


PS9, Line 255: pending_destruction
> maybe: destroy_after_write (or destroy_after_write)
Made the change. I'm on the fence a bit about the name, since there is a difference about who does the repinning versus the destruction (one happens in the callback, the other happens in DestroyPageInternal()).


http://gerrit.cloudera.org:8080/#/c/5584/9/be/src/runtime/bufferpool/buffer-pool.cc
File be/src/runtime/bufferpool/buffer-pool.cc:

Line 44:   Reset();
> okay, but why was this needed?
It was a bug - the code wasn't exercised - moving 'src' into an uninitialised BufferHandle hits a DCHECK a lot of the time.


Line 155:   RETURN_IF_ERROR(AllocateBufferInternal(client, len, &buffer));
> why don't we just use AllocateBuffer() here for lines 154,155,162?
That should work. I think there was some reason originally but it's no longer relevant after other changes to the code.


PS9, Line 176: ///
> //
Done


PS9, Line 228: AllocateBufferInternal
> is there a better name for that that would better explain why CleanPagesBef
I can't come up with a better name. It really just allocates the buffer without the reservation bookkeeping.


PS9, Line 239: decrease
> maybe 'delta'?
Done


Line 242:     int64_t to_evict = decrease - len;
> shouldn't this be len - decrease? is it missing test coverage?
Yep that was a bug. I added a DCHECK to EvictCleanPages() that checks that it's non-negative, which was hit. I also added checks to the test that assert that the pages were actually evicted when expected (which should also have caught it), which caught some additional accounting bugs.


Line 331:     FreeBufferInternal(&buffer);
> it looks like FreeBufferInternal() adjusts buffer_bytes_remaining_... is th
That was one of the bugs I caught by adding the IsEvicted() check to the tests.


Line 333:   if (total_len < bytes_to_evict) {
> in what case can this happen?
Only if there's an accounting error. Obviously we should do everything possible to avoid this, but it seemed better to return an INTERNAL_ERROR status instead of DCHECKing, since there's some chance we'll get the accounting wrong.


Line 469:     // 'repin_after_write'.
> close parenthese.
Done


Line 496:   // return on error.
> why this comment? i don't see any place in this method where we don't just 
Yeah this comment wasn't really necessary.


Line 503:   BufferHandle buffer;
> why don't we need to reserve here?
The accounting is handled in Pin(), since it's incremented regardless of which code path we take in Pin().


Line 533:   RETURN_IF_ERROR(write_status_); // Propagate error if we can't evict pages.
> after reading more code I see why this is needed but the existing comment d
I updated the comment to explain why the error would have been set.


http://gerrit.cloudera.org:8080/#/c/5584/9/be/src/runtime/bufferpool/buffer-pool.h
File be/src/runtime/bufferpool/buffer-pool.h:

PS9, Line 257: between lists is atomic
> maybe clarify by saying: from a client list to the global clean page list i
Done


PS9, Line 263: list is atomic and there is not a
             :   /// window so that moving the page between lists is atomic
> garbled.
Done


PS9, Line 271: if not enough bytes could be evicted
> when can this happen? It should be guaranteed to succeed by the reservation
Good point. Yes it would indicate an accounting bug. It seemed worth doing this (instead of adding a DCHECK) so that we fail more gracefully in that case.


PS9, Line 306: Client
> I wonder if we should rename Client to ClientHandle and ClientImpl to Clien
I thought about that too. I'll go ahead and do it.


-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes