helifu 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) thanks for your detailed comments. 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 appropriat done. 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 done. http://gerrit.cloudera.org:8080/#/c/12075/3/src/kudu/fs/log_block_manager-test.cc@1798 PS3, Line 1798: removing > Nit: removal done. 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" done. 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" done. 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 LogBlockDeletionT done. 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 done. 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 Commi can not agree any more ;) 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 cal done. 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 done. 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(). done. -- 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: Mon, 17 Dec 2018 12:20:53 +0000 Gerrit-HasComments: Yes
