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 6: > maybe that is not enough. please take a look at this case: two remaining > LogBlocks are deleted in different LogBlockDeletionTransaction concurrently, > then one will trigger hole punching and the other will trigger dead container > removal. but it is not safe to access the container for hole punching when > removing dead container is executed first. > i think the key point is that the container point in > 'all_containers_by_name_' is a raw point which will never be deleted after > startup. in other words, the container point is not safe. > i am going to use 'std::shared_ptr' and 'std::weak_ptr' to wrap it. > what do you think? That's a very good point. It's hard to come up with a robust solution that doesn't involve shared ownership for containers via std::shared_ptr. I don't think you need weak_ptr though. You could take mimic the FileCache behavior and perform dead container deletion in the container's destructor. So the sequence of events could be something like this: 1. A bunch of deletion transactions run in different threads, all affecting the same container. 2. All but one of them trigger hole punching on the container. The hole punch closures capture a shared ref to the container. 3. One triggers container deletion, which actually just marks the container as deleted and removes it from the global map. 4. The hole punch closures run, and punch holes (unnecessary, but safe). 5. When all the closures are done, the container's ref count is now 0. Its destructor runs, and the container is safely deleted from disk. Since wrapping up containers in std::shared_ptr may be an extensive change, would you mind creating a separate change for it, and then rebasing this change on top of it? -- 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: 6 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: Thu, 20 Dec 2018 02:06:39 +0000 Gerrit-HasComments: No
