Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/7656 )
Change subject: KUDU-2055 [part 1]: Add fs::BlockDeletionTransaction API ...................................................................... Patch Set 9: (6 comments) http://gerrit.cloudera.org:8080/#/c/7656/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/7656/9//COMMIT_MSG@7 PS9, Line 7: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of > I think the message needs to be on a single line for most systems (e.g. Git Done http://gerrit.cloudera.org:8080/#/c/7656/9//COMMIT_MSG@22 PS9, Line 22: abstraction 'BlockDeletionTransaction', and doesn't yet change any > In that case it may be better to change the title to something like 'Add fs Done http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager-test.cc@1109 PS9, Line 1109: forcing > force Done http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager.h@325 PS9, Line 325: Status CommitDeletedBlocks(std::vector<BlockId>* deleted) { > The 'deleted' out parameter is a little bit unusual here, in that it's only Yes, this is used in follow-up commit. Especially at TabletMetadata::DeleteOrphanedBlocks where requires to know which blocks were successfully deleted. http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager.h@333 PS9, Line 333: deleted->emplace_back(block); > Should the block be added even if it's status is NotFound? If the status is NotFound, we can consider it to be already deleted? And notify the caller about so. Or do you have some concern around it? http://gerrit.cloudera.org:8080/#/c/7656/9/src/kudu/fs/block_manager.h@342 PS9, Line 342: deleted_blocks_.clear(); > Shouldn't we clear regardless of error? Done -- To view, visit http://gerrit.cloudera.org:8080/7656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959 Gerrit-Change-Number: 7656 Gerrit-PatchSet: 9 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 22 Sep 2017 00:34:39 +0000 Gerrit-HasComments: Yes
