Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 4]: Coalesce hole punch for LBM ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.h@299 PS2, Line 299: Blocks belonging to read-only containers will not be deleted. > 'Blocks belonging to read-only containers will not be deleted. Done http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1167 PS2, Line 1167: one hole punc > s/hole punching/one hole punch Done http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1174 PS2, Line 1174: void AddBlock(const scoped_refptr<internal::LogBlock>& lb); > I think this would be more clear if it were named 'AddBlock'. 'Update' is Done http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1196 PS2, Line 1196: " > s/at/in Done http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1202 PS2, Line 1202: > * with the type Done http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1207 PS2, Line 1207: > Hoist this check/increment out of the loop and use IncrementBy Done http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1220 PS2, Line 1220: Status LogBlockDeletionTransaction::CommitDeletedBlocks( > Since there are separate implementations of the transaction types now, perh Yes, makes sense. Will do. http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1283 PS2, Line 1283: DCHECK(transaction); > Can this be simplified to I think only when multiple block deletions register to the same 'transaction'(which is L2556 and L1221), we will need ptr() here. Since L1221 already uses shared_from_this(). Maybe we can simplify it to transaction_ = transaction; if in L2556 also do the same. http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@2027 PS2, Line 2027: Status LogBlockManager::RemoveLogBlock( > This comment should be moved down to the read_only() check Done -- To view, visit http://gerrit.cloudera.org:8080/8162 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275 Gerrit-Change-Number: 8162 Gerrit-PatchSet: 3 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-Comment-Date: Fri, 29 Sep 2017 22:14:28 +0000 Gerrit-HasComments: Yes
