Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18255 )
Change subject: KUDU-3197 [tserver] optimal Schema's memory used, using std::shared_ptr ...................................................................... Patch Set 8: (7 comments) http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/diskrowset.cc@633 PS3, Line 633: const SchemaPtr schema_ptr = rowset_metadata_->tablet_schema(); > Do we need to make a shared_ptr here? I suspect we do yes. Done http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/ops/alter_schema_op.cc File src/kudu/tablet/ops/alter_schema_op.cc: http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/ops/alter_schema_op.cc@107 PS3, Line 107: schema_ptr)); > Why not pass the shared ptr? Otherwise we end up creating a new shared ptr Done http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/tablet.cc@548 PS3, Line 548: client_schema, > Why there are two styles, one is declear a new variable, like 'SchemaPtr sc Change it to: SchemaPtr schema_ptr = schema(); http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/tablet.cc@548 PS3, Line 548: client_schema, > +1, it's worth clarifying what the policy is for using shared_ptr vs raw po yes. op_state's schema_at_decode_time is unsafe. I would add a SchemaPtr to protect it. http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/tablet.cc@1616 PS3, Line 1616: SchemaPtr schema = std::make_shared<Schema>(new_schema); > How about use std::make_shared? Done http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/tablet_metadata.cc@940 PS3, Line 940: } : > This may actually destruct the old schema, which could be expensive. Instea good http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tserver/tserver_path_handlers.cc File src/kudu/tserver/tserver_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tserver/tserver_path_handlers.cc@368 PS3, Line 368: tmeta->partition > Don't we need to make a schema_ptr here? Done -- To view, visit http://gerrit.cloudera.org:8080/18255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic284dde108c49130419d876c6698b40c195e9b35 Gerrit-Change-Number: 18255 Gerrit-PatchSet: 8 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Wed, 02 Mar 2022 04:14:56 +0000 Gerrit-HasComments: Yes
