Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12308 )
Change subject: KUDU2665: LBM may delete containers with live blocks ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/12308/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12308/2//COMMIT_MSG@7 PS2, Line 7: KUDU2665: LBM may delete containers with live blocks Nit: add a dash between 'KUDU' and '2665'. http://gerrit.cloudera.org:8080/#/c/12308/2/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/12308/2/src/kudu/fs/log_block_manager-test.cc@1873 PS2, Line 1873: MetricRegistry registry; : scoped_refptr<MetricEntity> entity = METRIC_ENTITY_server.Instantiate(®istry, "test"); : ASSERT_OK(ReopenBlockManager(entity)); If you're not testing metrics, I don't think you need to do this. http://gerrit.cloudera.org:8080/#/c/12308/2/src/kudu/fs/log_block_manager-test.cc@1877 PS2, Line 1877: // Create a bunch of blocks. Nit: new code should use BlockCreationTransactions to create blocks. http://gerrit.cloudera.org:8080/#/c/12308/2/src/kudu/fs/log_block_manager-test.cc@1895 PS2, Line 1895: : // Lower the max container size, now the container is full. : FLAGS_log_container_max_size = 1; Doing this here feels a little weird. It's not something you could do in production (in that the flag can't be reconfigured at runtime), and by doing it after Finalize() the container lands on the "available container" list. http://gerrit.cloudera.org:8080/#/c/12308/2/src/kudu/fs/log_block_manager-test.cc@1920 PS2, Line 1920: CHECK_OK(bm_->OpenBlock(block_id, &block)); ASSERT_OK. http://gerrit.cloudera.org:8080/#/c/12308/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12308/2/src/kudu/fs/log_block_manager.cc@554 PS2, Line 554: // Tries to mark the container as 'dead'. It can only be set successfully : // once, and it is deleted when the container goes out of scope. Nit: "Tries to mark the container as 'dead', which means it will be deleted when it goes out of scope. Can only be set dead once." -- To view, visit http://gerrit.cloudera.org:8080/12308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I894f32b1c164ae7770c92171850edd167dfaf8ad Gerrit-Change-Number: 12308 Gerrit-PatchSet: 2 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Thu, 31 Jan 2019 19:36:11 +0000 Gerrit-HasComments: Yes
