impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4114: Port BufferedBlockMgr tests to buffer pool
Date Fri, 07 Apr 2017 00:04:50 GMT
Dan Hecht has posted comments on this change.

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

Patch Set 5:

File be/src/runtime/bufferpool/

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.

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

To view, visit
To unsubscribe, visit

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

View raw message