Adar Dembo 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? Also elsewhere where this pattern is repeated. 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? 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; what's the use case for it to add more entries to an existing vector? 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 we gate the call to NewDeletionTransaction() on FLAGS_repair instead. 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 unordered_set after CommitDeletedBlocks() before doing this inner lookup? That should bring it back to O(n). -- 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: Fri, 06 Oct 2017 23:14:55 +0000 Gerrit-HasComments: Yes
