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 12: (6 comments) http://gerrit.cloudera.org:8080/#/c/12075/12/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/12075/12/src/kudu/fs/log_block_manager-test.cc@311 PS12, Line 311: 0, 0, 1, 0, 0, 0, 0 > nit: can you add a comment to note what each metric means. e.g., /*METRIC_l Sorry, that's not a good solution i think. My motivation for the changes here is to eliminate and simplify redundant code, so i would rather rollback the modification than add comments. What do you think? :p http://gerrit.cloudera.org:8080/#/c/12075/12/src/kudu/fs/log_block_manager-test.cc@1696 PS12, Line 1696: TestDeleteDeadContainersByDeletionTransaction > Can you also add a test for two log blocks refer to one log block container Done http://gerrit.cloudera.org:8080/#/c/12075/12/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12075/12/src/kudu/fs/log_block_manager.cc@372 PS12, Line 372: ~LogBlockContainer(); > Add a comment to describe what happens in this destructor. Done http://gerrit.cloudera.org:8080/#/c/12075/12/src/kudu/fs/log_block_manager.cc@760 PS12, Line 760: bool metadata_missing = false; : bool data_missing = false; > What was the motivation for the changes here? Doesn't seem like the behavio The motivation here is to support deleting incomplete files directly, otherwise opening this type of container will fail and need manual intervention. And i believe this will happen occasionally when rebooting cluster. As was said in last comments, it's not friendly for kudu newbee. Well, i rollback this modification. :( http://gerrit.cloudera.org:8080/#/c/12075/12/src/kudu/fs/log_block_manager.cc@1342 PS12, Line 1342: // It's possible for multiple deletion transactions to end up here. For : // example, if one transaction deletes the second to last block in a : // container and the second deletes its last block, the destructors of : // both transactions could run at a time that container's internal : // bookkeeping reflects the deadness of the container. > Can you add one more sentence to explain this situation is guarded by TrySe Done http://gerrit.cloudera.org:8080/#/c/12075/12/src/kudu/fs/log_block_manager.cc@2366 PS12, Line 2366: // TODO(adar): this should be reported as an inconsistency once dead : // container deletion is also done in real time. Until then, it would be : // confusing to report it as such since it'll be a natural event at startup. > Should this be updated? 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: 12 Gerrit-Owner: helifu <hzhel...@corp.netease.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: helifu <hzhel...@corp.netease.com> Gerrit-Comment-Date: Thu, 10 Jan 2019 07:29:57 +0000 Gerrit-HasComments: Yes
