Andrew Wong 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 5: (5 comments) Sorry for the delay in review. Thanks for the contribution! Looking good so far :) 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 Schema* schema = rowset_metadata_->tablet_schema().get(); Do we need to make a shared_ptr here? I suspect we do 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.get()) Why not pass the shared ptr? Otherwise we end up creating a new shared ptr in this call anyway. 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 +1, it's worth clarifying what the policy is for using shared_ptr vs raw pointers. For instances where the function expects a reference, I'm not sure it's safe to just pass the pointer, since it's possible the underlying schema is destroyed, since this call site doesn't keep a shared_ptr around. It'd be safer if we did something like: SchemaPtr shared_schema = schema(); RowOperationsPBDecoder dec(&op_state->request()->row_operations(), client_schema, schema_ptr.get(),... Or perhaps you're relying on the fact that we locked the schema at L538? If so, it's still worth in a comment, so other developers in this area of code better understand the nuances. 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: std::lock_guard<LockType> l(data_lock_); : SetSchemaUnlocked(schema, version); This may actually destruct the old schema, which could be expensive. Instead, maybe we should implement some SwapSchemaUnlocked() so the destruction doesn't happen while data_lock_ is held. void TabletMetadata::SetSchema(SchemaPtr schema, uint32_t version) { // In case this is the last reference to the schema, destruct the pointer // outside the lock. SchemaPtr old_schema; { lock_guard<LockType> l(data_lock_); SwapSchemaUnlocked(std::move(schema), version, &old_schema); } } void TabletMetadata::SwapSchemaUnlocked(SchemaPtr schema, uint32_t version, SchemaPtr* old_schema) { *old_schema = std::move(schema_); schema_ = std::move(schema); schema_version_ = version; } 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->schema() Don't we need to make a schema_ptr here? -- 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: 5 Gerrit-Owner: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <acelyc1112...@gmail.com> Gerrit-Comment-Date: Fri, 25 Feb 2022 03:09:18 +0000 Gerrit-HasComments: Yes
