Adar Dembo 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: (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 total number of deletions since startup. By comparing it with holes_punched, we can see how effective the coalescing behavior is. Actually, let's add 'blocks_deleted' and 'blocks_created' as generic block manager metrics (see block_manager_metrics.{cc,h}), since they're not specific to the LBM. http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@14 PS3, Line 14: operation operations http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@17 PS3, Line 17: vaule value http://gerrit.cloudera.org:8080/#/c/8162/3//COMMIT_MSG@17 PS3, Line 17: coalescing hole punch works as expected by checking the vaule of punching 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: class BlockDeletionTransaction { In part 3 I left a comment about how it'd be nice for these transaction classes to be purely virtual. If you agree, I think this bit of refactoring should be moved to part 3. 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: std::shared_ptr<BlockDeletionTransaction> deletion_transaction = Add a using:: for this. 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_gauge_uint64(server, log_block_manager_holes_punched, Because the number of holes punched only ever increases, this should be a counter not a gauge. See "Gauge vs. Counter" in metrics.h for more information. http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@129 PS3, Line 129: "Blocks Under Management", Update. http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@130 PS3, Line 130: kudu::MetricUnit::kBlocks, Update. http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@131 PS3, Line 131: "Number of data blocks currently under management"); Update. http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@182 PS3, Line 182: scoped_refptr<AtomicGauge<uint64_t>> holes_punched; Please separate from the previous two metrics with a blank line, since it's not an "under management" metric. http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@229 PS3, Line 229: to with http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@230 PS3, Line 230: gets is http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@246 PS3, Line 246: that this block has been registered to with which this block has been registered. http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1208 PS3, Line 1208: if (container->block_manager()->metrics()) { : container->block_manager()->metrics()->holes_punched->IncrementBy( : entry.second.size()); : } For this to be more accurate: 1. Defer it to ContainerDeletionAsync, so that the change in metric values reflect the time that it actually takes to process the hole punches. 2. Only do it if hole punching actually succeeds. http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1224 PS3, Line 1224: Status s = lbm_->RemoveLogBlock(block, shared_from_this()); Would be better if: 1. shared_from_this() were called once; no need to call it for every iteration in the loop. 2. RemoveLogBlock() was actually RemoveLogBlocks() to avoid a spinlock acquisition/release with each loop iteration. http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@1286 PS3, Line 1286: transaction_->AddBlock(this); The call sequence for deleting blocks is long and involves a lot of class traversal: 1. LogBlockManager::DeletionTransaction() to get a new transaction. Also calls BlockDeletionTransaction and LogBlockDeletionTransaction constructors. 2. BlockDeletionTransaction::AddDeletedBlock to mark a block as to-be-deleted. 3. LogBlockDeletionTransaction::CommitDeletedBlocks to effect the deletions in-memory and to set them up to be deleted on disk. This calls LogBlockManager::RemoveLogBlock, which calls LogBlock::RegisterDeletion, which calls LogBlockDeletionTransaction::AddBlock. Quite the back-and-forth. 4. ~LogBlockDeletionTransaction to effect the on-disk deletions. It's step 3 in particular that I find difficult to grok. Here are some ideas for simplification: - Make BlockCreationTransaction and BlockDeletionTransaction pure virtual. That eliminates at least one hop, since these interfaces won't have any code in them. - Change deletion transaction semantics to be two-step instead of three-step. Right now the three distinct steps are: 1) AddDeletedBlock to show that you intend to delete a block. 2) CommitDeletedBlocks on the entire transaction to do the in-memory deletions. 3) When the transaction goes out of scope, the on-disk data is deleted. Perhaps step #2 could be done as part of step #3? - Consider adding another patch before this one to force all deletions through BlockDeletionTransaction, thus allowing you to eliminate BlockManager::DeleteBlock. By doing so, you might be able to refactor the LBM more thoroughly because various internal deletion-related functions won't need to support multiple callers. http://gerrit.cloudera.org:8080/#/c/8162/3/src/kudu/fs/log_block_manager.cc@2550 PS3, Line 2550: // TODO(adar): can be more efficient (less fs work and more space reclamation : // in case of misaligned blocks) via hole coalescing first, but this is easy. Remove/update. -- 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: Mon, 02 Oct 2017 22:26:44 +0000 Gerrit-HasComments: Yes
