Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18198 )
Change subject: WIP KUDU-3197 [tserver] optimal Schema's memory used, using std::shared_ptr ...................................................................... Patch Set 14: (7 comments) Fix them. close it and re-commit it. http://gerrit.cloudera.org:8080/#/c/18198/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18198/5//COMMIT_MSG@18 PS5, Line 18: wangningito has proposed a simple solution at https://gerrit.cloudera.org/c/16508/. : And I continue his work, and the detail at: https://gerrit.cloudera.org/c/18098/, : But the changes has been thought too much, and code reviewers advise changing from : scoped_refptr<Schema> to std::shared_ptr. > I also left a comment on that ticket mentioning a pretty important detail: ok, I will study it again. http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/client/client-test.cc@4713 PS5, Line 4713: ColumnSchema col_schema = sch > In general, the old code just made a copy of the Schema; if you want, you c ok http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/common/generic_iterators.cc@518 PS5, Line 518: // Initialized during Init. : unique_ptr<Schema> > The problem statement for this ticket only refers to the schemas owned by t yes. I will restore it. http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/consensus/log.cc@771 PS5, Line 771: scoped_refptr<Log> new_log(new Log(std::move(optio > Why not pass the smart pointer directly? Doesn't this mean that we now have Log no need change, I restore it. http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/master/catalog_manager.cc@2469 PS5, Line 2469: ColumnId* next_col_id) { : const SchemaPB& current_schema_pb = current > Again, it seems these changes are unrelated to the schemas referred to in K Yes. I will remove it http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/master/sys_catalog.cc@342 PS5, Line 342: schema, pa > Seems like it would be less intrusive to have call-sites use Schema as they You are right. restore it. http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/18198/5/src/kudu/tablet/delta_tracker.cc@224 PS5, Line 224: opts.projection = projection; > Without this patch, it seems like this projection is already reference coun opts.projection is const Schema* before. So I think we should protect the schema. -- To view, visit http://gerrit.cloudera.org:8080/18198 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I586e3a67791de4006c3479b39f3222cbf93fac28 Gerrit-Change-Number: 18198 Gerrit-PatchSet: 14 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Fri, 18 Feb 2022 13:42:03 +0000 Gerrit-HasComments: Yes
