Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 5]: Coalesce hole punch for LBM ...................................................................... Patch Set 4: (18 comments) http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@13 PS3, Line 13: It also adds a new metric 'holes_punched' in log block manager to track > Let's also add a new counter for 'blocks_deleted', which will track the tot Done http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@14 PS3, Line 14: operation > operations Done http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@17 PS3, Line 17: > value Done http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@17 PS3, Line 17: > punching Done http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/block_manager.h@323 PS3, Line 323: > In part 3 I left a comment about how it'd be nice for these transaction cla Right, I agree with it and updated part3. http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager-test.cc@347 PS3, Line 347: FLAGS_log_container_max_size = 10LU * 1024 * 1024 * 1024; > Add a using:: for this. Done http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@128 PS3, Line 128: METRIC_DEFINE_counter(server, log_block_manager_holes_punched, > Because the number of holes punched only ever increases, this should be a c Done http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@129 PS3, Line 129: "Number of Holes Punched", > Update. Done http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@130 PS3, Line 130: kudu::MetricUnit::kLogBlockContainers, > Update. Done http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@131 PS3, Line 131: "Number of hole punching operations"); > Update. Done http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@182 PS3, Line 182: > Please separate from the previous two metrics with a blank line, since it's Done http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@229 PS3, Line 229: > with Done http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@230 PS3, Line 230: > is Done http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@246 PS3, Line 246: > with which this block has been registered. Done http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1208 PS3, Line 1208: public: : explicit LogBlockDeletionTransaction(LogBlockManager* lbm) : : lbm_(lbm) { : } > For this to be more accurate: I agree with you it will be more accurate. But the only issue here is doing so will make LogBlockManagerTest::MetricsTest hard to work, because we don't know when the hole punching operation actually takes place? http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1224 PS3, Line 1224: // Block <offset, offset + length> pair. > Would be better if: Done http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1286 PS3, Line 1286: } else { > The call sequence for deleting blocks is long and involves a lot of class t Updated to make BlockCreationTransaction and BlockDeletionTransaction pure virtual, and added another patch to force all deletions through BlockDeletionTransaction. But I think for the second recommendation, we probably cannot merge #2 with #3? Since we need to do in-memory deletions separately from #3 to have the transaction goes out of scope (free all the references to the deleted LogBlocks). Given the current updates, now LogBlockDeletionTransaction::CommitDeletedBlocks doesn't have back-and-forth call chain. It only calls LogBlockManager::RemoveLogBlock and LogBlock::RegisterDeletion separately. http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@2550 PS3, Line 2550: : // Technically we've "repaired" the inconsistency if the truncation > Remove/update. 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: 4 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-Comment-Date: Thu, 05 Oct 2017 17:25:26 +0000 Gerrit-HasComments: Yes
