Hao Hao 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//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12075/12//COMMIT_MSG@9 PS12, Line 9: LBM supports deleting Can you rephrase the sentence to say this patch intends to support deleting dead containers? This sounds like it is already supported today. 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_log_block_manager_containers = */ 1. Same below. 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 and concurrently delete both and verify the metric looks right? 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. 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 TrySetDead()? 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? -- 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: Wed, 09 Jan 2019 23:18:10 +0000 Gerrit-HasComments: Yes
