Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12075 )
Change subject: KUDU-2636: LBM supports deleting dead and full containers ...................................................................... Patch Set 3: (11 comments) Looks good! Hao did some recent work in the log block manager; Hao, could you also take a look? http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager-test.cc@1754 PS3, Line 1754: TestDeleteDeadContainersAtPunchHole Probably "TestDeleteDeadContainersByDeletionTransaction" is more appropriate now. http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager-test.cc@1796 PS3, Line 1796: } Can you add a variant of this test that opens the block for reading before deleting it, tests that after the BlockDeletionTransaction goes out of scope, the block (and its container) is still alive, and only when the opened block goes out of scope is the block (and the container) deleted? You should be able to reuse pretty much all of this code with some parameterization. http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager-test.cc@1798 PS3, Line 1798: removing Nit: removal http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager.cc@141 PS3, Line 141: "Number of Dead Block Containers", "Number of Dead Block Containers Deleted" http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager.cc@143 PS3, Line 143: "Number of dead block containers that were deleted"); "Number of full (but dead) block containers that were deleted" http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager.cc@1237 PS3, Line 1237: // Given the shared ownership of LogBlockDeletionTransaction, at this point : // all registered blocks should be destructed. Thus, coalescing deletions : // for blocks that are contiguous in a container and schedules hole punching. Can you update this comment to reflect the changes to the LogBlockDeletionTransaction destructor? http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager.cc@1269 PS3, Line 1269: std::unordered_map<DataDir*, std::vector<std::string>> containers_by_dir; Don't need std:: prefixes for the new code; we're "using ..." that stuff at the top of the file (and if we're not for a particular class, add a "using::" definition for it). http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager.cc@1273 PS3, Line 1273: // Filter out the containers that are full and dead. FWIW, I think it'd be cleaner to identify full and dead containers in CommitDeletedBlocks (after the call to RemoveLogBlocks). Then, don't even call AddBlock() on them; just throw them into a new std::unordered_map of container names keyed by data dirs. Finally, in the destructor, you process the deleted_interval_map_ as before, then process the new unordered_map separately. http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager.cc@1946 PS3, Line 1946: void LogBlockManager::RemoveFullContainerUnlocked(const string& container_name) { Can you convert this into RemoveFullContainersUnlocked? Looks like both callers would benefit from that right now: it'd let you do one metrics decrement instead of a per-container decrement. http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager.cc@1968 PS3, Line 1968: // Invalidate the cache and remove from the disk. Please consolidate this with the equivalent code in Repair(). In particular: - We should WARN when a deletion fails. - We should trigger the disk failure handling path when a deletion fails. http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager.cc@1976 PS3, Line 1976: if (metrics()) { : metrics()->dead_containers_deleted->Increment(); : } You can pull this out of the loop and increment just once by names.size(). -- To view, visit http://gerrit.cloudera.org:8080/12075 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7d3672ae3c3dd9acd489120d653c44a641537f10 Gerrit-Change-Number: 12075 Gerrit-PatchSet: 3 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Sat, 15 Dec 2018 20:49:25 +0000 Gerrit-HasComments: Yes
