helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12121 )
Change subject: [fs]: wrapping up containers in scoped_refptr ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/12121/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12121/3/src/kudu/fs/log_block_manager.cc@233 PS3, Line 233: LogBlockContainerRefPtr container_; > This was my biggest concern with this change: in large deployments with mil The durations of 'dense_node-itest' before and after this change show little difference, but i know that it is not enough. Maybe i can have a try to test the impact on node startup next week. http://gerrit.cloudera.org:8080/#/c/12121/3/src/kudu/fs/log_block_manager.cc@333 PS3, Line 333: // LogBlockContainer are reference counted to simplify support for deletion : // with LogBlock deletion. > Nit: add an empty line just before and reword: Done http://gerrit.cloudera.org:8080/#/c/12121/3/src/kudu/fs/log_block_manager.cc@1248 PS3, Line 1248: // To keep using raw point as key in the unordered_map safely, : // it is important to save scoped_refptr of LogBlockContainer : // here. And, the reason why not saving scoped_refptr of LogBlock : // is that it will cause circular dependency issue. > You could use LogBlockContainerRefPtr as a key to an associative container Done http://gerrit.cloudera.org:8080/#/c/12121/3/src/kudu/fs/log_block_manager.cc@1584 PS3, Line 1584: // The owning container. Must outlive LogBlockRefPtr below. : LogBlockContainer* container_; > Just drop this altogether. There are several good reasons to do that: Done -- To view, visit http://gerrit.cloudera.org:8080/12121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c5c620014782b3d57dcbe047d0df73c949590c7 Gerrit-Change-Number: 12121 Gerrit-PatchSet: 3 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Fri, 04 Jan 2019 09:46:03 +0000 Gerrit-HasComments: Yes
