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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
