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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-HasComments: Yes
