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 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/fs/log_block_manager.cc@1179 PS1, Line 1179: // Though the result of deleting a data block is not the same as : // before when there are empty data blocks in the same container : // which is FULL, especially in the unit test, this design is still : // acceptable. I don't agree with this assessment. This approach suffers from the following problems: 1. It goes to disk unnecessarily to figure out if a container is dead. If we can safely call full(), why can't we call live_blocks() and compare it to 0? 2. By chaining to ContainerDeletionAsync, we can't "batch" container deletion. LogBlockDeletionTransaction allows for the client to specify a group of blocks to be deleted together (i.e. deleting an entire tablet). The LBM uses this to optimize deletion where it can. In the case of deleting containers that are no longer necessary, we can delete all of the container files, then do one SyncDir() call per parent directories, instead of a SyncDir() per container. 3. Furthermore, if a LogBlockDeletionTransaction is going to delete all of the blocks in a container, it'd be cheaper to recognize that and, rather than punching any holes, delete the container files outright. http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/fs/log_block_manager.cc@1966 PS1, Line 1966: void LogBlockManager::RemoveDeadFullContainer(const std::string& container_name) { Would be good to reuse/consolidate the file deletion code here with the equivalent in Repair(). http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/util/env.h File src/kudu/util/env.h: http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/util/env.h@648 PS1, Line 648: virtual Status SizeOnDisk(uint64_t* size) const = 0; Could you add a brief unit test to env-test to cover this new method? http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/12075/1/src/kudu/util/env_posix.cc@1299 PS1, Line 1299: // From stat(2): : // : // The st_blocks field indicates the number of blocks allocated to : // the file, 512-byte units. (This may be smaller than st_size/512 : // when the file has holes.) : *size = sbuf.st_blocks * 512; Can you reuse this block of code for RWFile::SizeOnDisk, using a decomposed static helper? The motivation here is understandability: it'd be good for readers to see that comment. -- 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: 1 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 12 Dec 2018 19:09:53 +0000 Gerrit-HasComments: Yes
