impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4114: Port BufferedBlockMgr tests to buffer pool
Date Fri, 07 Apr 2017 03:27:09 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4114: Port BufferedBlockMgr tests to buffer pool
......................................................................


Patch Set 5:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/6498/5/be/src/runtime/bufferpool/buffer-pool-test.cc
File be/src/runtime/bufferpool/buffer-pool-test.cc:

PS5, Line 1017: changes if there are multiple NUMA nodes
> how so?
Added some clarification.


Line 1066:   DestroyAll(&pool, &client, &extra_pages);
> why this?
We need to free them to pin the pages again. Reordered the flow here to make it a bit clearer.
Also switched to allocating buffers, since we don't need them to be pages.


Line 1069:   for (PageHandle& page : pages)
> nit: missing braces
Done


PS5, Line 1074: needed
> needed to?
Done


Line 1082: TEST_F(BufferPoolTest, DestroyDuringWrite) {
> comment summarizing the goal of this test
Done


PS5, Line 1136: writes
> which writes? the ones at line 1137, or do you mean the ones at 1149?
Restructured this code and comments abit. Made it clearer that it's future writes.


PS5, Line 1142: DestroyAll
> why does this happen after 1141? is that relevant or can this happen immedi
It's to force eviction of the pages. I switched to using buffers and added a comment to make
that a bit clearer.


Line 1201:   Status error = pool.AllocateBuffer(&client, TEST_BUFFER_LEN, &tmp_buffer);
> why does this result in an error? i thought we allocate the file space only
Yeah, basically we can't guarantee the invariant if a write failed.


Line 1410: // Test that the buffer pool can still create pages when no scratch is present.
> what happens if you then unpin? do we test that case?
Extended this test to check the behaviour. Found a minor bug where we're returning the wrong
error code from TmpFileMgr.


Line 1429: // setting scratch_limit = 0.
> same
Added code to exercise it. This is a bit different since we'll actually disable spilling query-wide
in this case, but it seem worth exercising the code.


PS5, Line 1525: Test that randomly issues CreatePage(), Pin(), Unpin(), and DestroyPage()
calls.
> garbled
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb0221e8bea6f3b23b62d5094634d97562295ea3
Gerrit-PatchSet: 5
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

Mime
View raw message