Adar Dembo has posted comments on this change. Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks ......................................................................
Patch Set 6: (9 comments) Here's my first pass. I'll do a deeper pass after you've addressed these comments. http://gerrit.cloudera.org:8080/#/c/7656/6//COMMIT_MSG Commit Message: PS6, Line 14: hole punching holes punched Line 15: will be reduced from one per block to one per log block container. That's assuming that the individual holes are all contiguous. In the worst case, there's still one hole punched per block, right? PS6, Line 17: compaction I actually think tablet deletion is a better example, since that'll delete a great many blocks which were often written contiguously. PS6, Line 19: serial series http://gerrit.cloudera.org:8080/#/c/7656/6/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: PS6, Line 165: deletion deletions Line 170: class BlockDeletionTransaction : public RefCountedThreadSafe<BlockDeletionTransaction> { Would prefer if this used std::shared_ptr rather than scoped_refptr. We generally prefer std::shared_ptr for new code, unless there's a good reason not to. Also, can you move this down to just after BlockCreationTransaction? Line 180: virtual std::vector<BlockId> MarkDeletions(std::vector<BlockId> blocks) = 0; I think this should be structured like BlockCreationTransaction in that: 1. Deletions are "added" to the ongoing tracking apparatus. 2. The transaction is "committed" when it is ready. PS6, Line 259: // Creates a new block deletion transaction block, which can be used : // to coalesce a group of block deletions. : virtual scoped_refptr<BlockDeletionTransaction> CreateDeletionTransaction() = 0; Can we start with BlockDeletionTransaction behaving more like BlockCreationTransaction? That is, created separately? Later on we can change the semantics of both transaction classes such that they're created from a block manager. Line 288: class BlockCreationTransaction { Could you do this rename as part of a separate patch? It's clearly a trivial patch and would make it easier to review the new BlockDeletionTransaction. -- To view, visit http://gerrit.cloudera.org:8080/7656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master 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-HasComments: Yes
