Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7656 )
Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks ...................................................................... Patch Set 9: (4 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. GitHub) to handle it correctly. It's better to be a bit over the limit than wrapped in this case 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::BlockDeletionTransaction API', and save the current title for the commit which does coalescing. 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 used in the case of an error. Is having the list of deleted blocks in the error case actionable (is it being used in a follow-up commit)? Would it be sufficient just to set the number of deleted blocks? 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? -- 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 <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 21 Sep 2017 20:37:42 +0000 Gerrit-HasComments: Yes
