Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8219 )
Change subject: KUDU-2055 [part 4]: refactor BM to remove BlockManager::DeleteBlock ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/8219/2/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/8219/2/src/kudu/fs/block_manager-test.cc@463 PS2, Line 463: ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted)); > Can we also ASSERT that deleted.size() == 1? Done http://gerrit.cloudera.org:8080/#/c/8219/2/src/kudu/fs/block_manager-test.cc@955 PS2, Line 955: for (auto it = ids.begin(); it != ids.end();) { > Here we're deleting a set of blocks; can we do it in one transaction? Done http://gerrit.cloudera.org:8080/#/c/8219/2/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/8219/2/src/kudu/fs/log_block_manager-test.cc@750 PS2, Line 750: deleted.clear(); > Doesn't CommitDeletedBlocks take care of this? If not, I think it should; w Done http://gerrit.cloudera.org:8080/#/c/8219/2/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/8219/2/src/kudu/tools/tool_action_fs.cc@144 PS2, Line 144: shared_ptr<BlockDeletionTransaction> deletion_transaction = > Oh, this is why you removed the read-only checks in the previous patch. Can Done http://gerrit.cloudera.org:8080/#/c/8219/2/src/kudu/tools/tool_action_fs.cc@168 PS2, Line 168: auto iter = std::find(deleted.begin(), deleted.end(), entry.block_id); > This is an O(n^2) operation. How about converting 'deleted' into an unorder Done -- To view, visit http://gerrit.cloudera.org:8080/8219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ied2ab44f33bce29706f8f41acc3d468582edad6e Gerrit-Change-Number: 8219 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 11 Oct 2017 01:01:35 +0000 Gerrit-HasComments: Yes
