[Impala-ASF-CR] IMPALA-4114: Port BufferedBlockMgr tests to buffer pool

["Dan Hecht (Code Review)" <ge...@cloudera.org>]

Dan Hecht 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?


Line 1066:   DestroyAll(&pool, &client, &extra_pages);
why this?


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


PS5, Line 1074: needed
needed to?


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


PS5, Line 1136: writes
which writes? the ones at line 1137, or do you mean the ones at 1149?


PS5, Line 1142: DestroyAll
why does this happen after 1141? is that relevant or can this happen immediately after 1139? the code order made me wonder what's going on here.


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 when doing the file write. oh, I guess we pick up the error asynchronously in this case (and guaranteed to do so since we hit the reservation limit)?


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?


Line 1429: // setting scratch_limit = 0.
same


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


-- 
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 <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-HasComments: Yes