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 6: (18 comments) http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc@88 PS5, Line 88: // Block manager metrics. > Nit: separate with a space. Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc@257 PS5, Line 257: static void CheckGaugeMetric(const scoped_refptr<MetricEntity>& entity, > It's getting too hard to understand what each call to this means. How about Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc@340 PS5, Line 340: {1, &METRIC_log_block_manager_blocks_under_management}, > Would it be interesting to test the metrics after this but before the trans Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.h@308 PS5, Line 308: scoped_refptr<internal::LogBlock>* lb); > Doesn't LogBlock have a block ID member? Why do you need the first argument Done 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@130 PS5, Line 130: kudu::MetricUnit::kLogBlockContainers, > The unit is actually "holes". You may have to add a new MetricUnit. Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@131 PS5, Line 131: "Number of log block containers"); > "Number of holes punched since service start" Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1211 PS5, Line 1211: ionTransaction, > deleted_block_map_ Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1219 PS5, Line 1219: > This is only used in one place, so don't bother with a typedef. Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1224 PS5, Line 1224: // Add the given block that needs to be deleted to 'deleted_interval_map_', > These aren't blocks though, right? It's more like a 'deleted_interval_map_' Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1253 PS5, Line 1253: container->ExecClosure(Bind(&LogBlockContainer::ContainerDeletionAsync, > What should we do if this fails? Maybe we should coalesce in CommitDeletedB Yeah, we should launch the hole punch operation for this container and log it in case of failure here. But this scenario should never happen, as block length should >= 0 (the only failure case for CoalesceIntervals()). So I think it should be fine to keep it here. http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1257 PS5, Line 1257: } > IIUC, this arg is only here because friendship with a free function is toug Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1266 PS5, Line 1266: > Oh I see, you moved the checks from NewDeletionTransaction to here. I'd pre Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1269 PS5, Line 1269: > Nit: missing a space here Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1270 PS5, Line 1270: if (!s.ok()) { : if (first_failure.ok()) first_failure = s; : } else { : deleted->emplace_back(lb->block_id()); : // Register the block to be hole punched if metadata recording : // is successful. : lb->RegisterDeletion(transaction); : AddBlock(lb); : : if (lbm_->metrics_) { : lbm_->metrics_->generic_metrics.total_blocks_deleted->Increment(); : } : } : } : > Having one class grab the internal lock of another class is a code smell an Done 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) { > Hmm, I'd really like to eliminate DeleteBlock() altogether, as that would s Sorry, I do not quite follow why do we need to remove DoCloseBlocks() here in this patch? Yeah, there is no reason stop us from removing DeleteBlock() here. I think I probably just missed that. http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1897 PS5, Line 1897: > Nit: it's not a container in this code path. Done http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@2604 PS5, Line 2604: > belonging Done 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, : > It seems like these two are always called together. Perhaps they can be com Yeah, previously I combined them. But you pointed out that there are circle reference between LogBlockDeletionTransaction and LogBlock in LogBlock::RegisterDeletion. So I separate it out. Do you think that is still a problem? Or you mean combine them in another way? -- 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 <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, 16 Oct 2017 06:16:18 +0000 Gerrit-HasComments: Yes
