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 1: (3 comments) Thanks for your comments.@adar 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 followin Agree with point 1 and 3. But to point 2, i have a question, is it necessary to call SyncDir() after deleting the container? In my opinion it's ok that let the file system flush dirty data to disk.Then, the 'batch' containers deletion is unnecessary. 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? rollback. 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 rollback. -- 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-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Thu, 13 Dec 2018 03:40:11 +0000 Gerrit-HasComments: Yes
