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 5: (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. http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc@257 PS5, Line 257: static void CheckLogMetrics(const scoped_refptr<MetricEntity>& entity, It's getting too hard to understand what each call to this means. How about: 1. Add a function that given two sets (one of counter prototypes and one of gauge prototypes), tests them for the value 0. 2. Add a function that tests an individual counter prototype for a specific value. 3. Add a function that tests an individual gauge prototype for a specific value. Maybe that will be more clear without increasing the size of MetricsTest too much? http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager-test.cc@340 PS5, Line 340: ASSERT_OK(deletion_transaction->CommitDeletedBlocks(&deleted)); Would it be interesting to test the metrics after this but before the transaction goes out of scope? 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: const scoped_refptr<internal::LogBlock>& lb); Doesn't LogBlock have a block ID member? Why do you need the first argument? 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. http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@131 PS5, Line 131: "Number of hole punching operations"); "Number of holes punched since service start" http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1211 PS5, Line 1211: deleted_block_map deleted_block_map_ http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1219 PS5, Line 1219: // Map used to aggregate BlockInterval instances across containers. This is only used in one place, so don't bother with a typedef. http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1224 PS5, Line 1224: BlockDataByContainerMap deleted_block_map_; These aren't blocks though, right? It's more like a 'deleted_interval_map_'. http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1253 PS5, Line 1253: CoalesceIntervals<int64_t>(&entry.second); What should we do if this fails? Maybe we should coalesce in CommitDeletedBlocks() instead? http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1257 PS5, Line 1257: container->block_manager()->metrics(), IIUC, this arg is only here because friendship with a free function is tough, right? What if ContainerDeletionAsync were a LogBlockContainer method? Would that address this issue? http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1266 PS5, Line 1266: CHECK(!lbm_->read_only_); Oh I see, you moved the checks from NewDeletionTransaction to here. I'd prefer the other way around; better to catch a misuse of a read-only fsmanager early. http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1269 PS5, Line 1269: > Nit: missing a space here Also, why do you need this pair? The LogBlock has a block ID in it. http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1270 PS5, Line 1270: { : std::lock_guard<simple_spinlock> l(lbm_->lock_); : for (const auto &block_id : deleted_blocks_) { : scoped_refptr<LogBlock> lb; : Status s = lbm_->RemoveLogBlockUnlocked(block_id, &lb); : // If we get NotFound, then the block was already deleted. : if (!s.ok() && !s.IsNotFound()) { : if (first_failure.ok()) first_failure = s; : } else if (s.ok()) { : blocks.emplace_back(block_id, lb); : } else { : deleted->emplace_back(block_id); : } : } : } Having one class grab the internal lock of another class is a code smell and should be avoided unless absolutely necessary. In this case, surely this could be a private LogBlockManager function? http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1435 PS5, Line 1435: return container_->block_manager()->DeleteBlock(id()); Hmm, I'd really like to eliminate DeleteBlock() altogether, as that would simplify your current patch. Remind me why we didn't remove DoCloseBlocks() and DeleteBlock() from here? http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@1897 PS5, Line 1897: container Nit: it's not a container in this code path. http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@2604 PS5, Line 2604: // holes belong to the same container can be coalesced. belonging http://gerrit.cloudera.org:8080/#/c/8162/5/src/kudu/fs/log_block_manager.cc@2610 PS5, Line 2610: b->RegisterDeletion(shared_transaction); : transaction->AddBlock(b); It seems like these two are always called together. Perhaps they can be combined into one function? -- 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: 5 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: Fri, 06 Oct 2017 23:45:27 +0000 Gerrit-HasComments: Yes
