Adar Dembo has posted comments on this change. Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks ......................................................................
Patch Set 8: (10 comments) http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: Line 1102: Status s = deletion_transaction.CommitDeletedBlocks(&deleted_blocks); ASSERT_OK on this? http://gerrit.cloudera.org:8080/#/c/7656/8/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: PS8, Line 321: the list of IDs of : // block that can be successfully deleted. the list of block IDs that were successfully deleted. PS8, Line 324: that has been seen, if there is any that was seen, if any Line 326: Status status; How about calling this "first_failure"? PS8, Line 329: then the block could be already successfully : // deleted. then the block was already deleted Line 331: if (!s.ok() && !s.IsNotFound() && !status.ok()) { This should be status.ok(), right? Not !status.ok()? Line 334: deleted->emplace_back(block); Seems like you could end up here if !status.ok() but e.g. s.IsIOError(). PS8, Line 339: CloneAndAppend We typically combine statuses by prepending, not appending. PS8, Line 340: . Don't terminate status messages with periods since they're often combined with other statuses, at which point they're separated with colons. PS8, Line 339: the number of blocks could " : "not be deleted Should be something like "only deleted $0 blocks, first failure" Though you should produce this in a test and see what it looks like. -- 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: 8 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
