Adar Dembo 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 6: (13 comments) http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc@284 PS6, Line 284: ASSERT_NO_FATAL_FAILURE(CheckLogMetrics(entity, Since you're already updating all of this, please switch to the shorter NO_FATALS macro. http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc@382 PS6, Line 382: { {0, &METRIC_log_block_manager_holes_punched}, This might be flaky; the hole punch can happen as soon as deletion_transaction goes out of scope. http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager-test.cc@427 PS6, Line 427: { {1, &METRIC_log_block_manager_holes_punched}, This could be flaky, see above. http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1435 PS5, Line 1435: Status LogWritableBlock::AppendV(ArrayView<const Slice> data) { > Sorry, I do not quite follow why do we need to remove DoCloseBlocks() here My working theory is that much of the complexity of this patch is because we have to support two "delete block" code paths: one that's transaction-based, and one that isn't. AFAICT, this is the last place that uses DeleteBlock(), and it calls it (and DoCloseBlocks()) in order to fully delete a block on abort. I remembered that Dan, you, and I discussed this behavior in the past and I was wondering if you remembered why we didn't eliminate the DoCloseBlocks() and DeleteBlock() calls back then. http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@2610 PS5, Line 2610: internal::LogBlockContainer* container = FindPtrOrNull(containers_by_name, : > Yeah, previously I combined them. But you pointed out that there are circle I meant have one function that performs both operations. But, such a function would need friendship into either LogBlockDeletionTransaction or LogBlock, so perhaps it's not worth it. http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@1151 PS6, Line 1151: metrics_->holes_punched->Increment(); Only if PunchHole actually succeeded though, right? http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@1249 PS6, Line 1249: WARN_NOT_OK(CoalesceIntervals<int64_t>(&entry.second), I think CHECK_OK() is more appropriate if failure means some sort of bug on our part. http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@1269 PS6, Line 1269: Status s = lbm_->RecordDeletion(lb); Can this be taken care of by RemoveLogBlocks() too? Another way to look at this: as compared to "part 4", this patch should change CommitDeletedBlocks() to call a new DeleteBlocks() instead of the old DeleteBlock(). I can't see a reason why all of this work can't be done by a single DeleteBlocks() method, possibly decomposed into private LBM methods if it helps readability. The only catch is that RegisterDeletion/AddBlock need to be made available for Repair(), which is a little different as it doesn't delete from in-memory maps or from on-disk state. http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2063 PS6, Line 2063: log_blocks->emplace_back(lb); std::move(lb) here. http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2079 PS6, Line 2079: scoped_refptr<internal::LogBlock> Make this a cref rather than a copy so you don't need to ref the LogBlock. http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2112 PS6, Line 2112: Status LogBlockManager::RecordDeletion(const scoped_refptr<LogBlock>& lb) { Any particular reason this isn't part of RemoveLogBlocks? Seems like there'd be http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/fs/log_block_manager.cc@2594 PS6, Line 2594: shared_ptr<LogBlockDeletionTransaction> shared_transaction = : transaction->shared_from_this(); What's this for? 'transaction' is already behind a shared_ptr. Also, why doesn't this code path call NewDeletionTransaction to make the transaction? http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/8162/6/src/kudu/util/metrics.h@384 PS6, Line 384: kHoles, See the comment on L361; you need to update Name() too. -- 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: 6 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: Mon, 16 Oct 2017 23:31:36 +0000 Gerrit-HasComments: Yes
