Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage ...................................................................... Patch Set 4: (19 comments) http://gerrit.cloudera.org:8080/#/c/16043/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16043/4//COMMIT_MSG@10 PS4, Line 10: which nit: I'm a bit confused here; could you clarify what reads what from where? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h File src/kudu/transactions/status_tablet.h: PS4: To be more in-line with the name of the class, maybe rename these files into txn_status_tablet.{h,cc} ? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@19 PS4, Line 19: stdint.h nit: use <cstdint> instead? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@31 PS4, Line 31: nit: drop this extra line? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@41 PS4, Line 41: // Encapsulates the reading of state of the transaction status tablet. Is this for the typedef or for the class TransactionVisitor? I guess it's for the latter, so maybe move this comment one line below. http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@45 PS4, Line 45: Signal Do you think it's worth describing the semantics of various return statuses? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@58 PS4, Line 58: user STRING Would the value for this column be NULL for PARTICIPANT records? If so, maybe the 'user' column should not be here, but instead be in the metadata? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@71 PS4, Line 71: physically thread-safe I'm curious what 'physically thread-safe' means. Does it imply extra thread-safety guarantees compared with just 'thread-safe': could you add a bit more details here, please? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@90 PS4, Line 90: tablet_replica What is the relation between the schema of the tablet replica coming in here as a parameter and the schema statically generated in TxnStatusTablet::GetSchema()? Should the schemas be identical? If so, maybe add a CHECK/DCHECK in the constructor or add some other way to preserve the consistency? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@91 PS4, Line 91: : tablet_replica_(tablet_replica) {} nit: is it worth adding DCHECK(tablet_replica_) in the constructor? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc File src/kudu/transactions/status_tablet.cc: http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@20 PS4, Line 20: stddef.h nit: use <cstddef> ? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@72 PS4, Line 72: & Is the reference essential here? Why not simply 'const string' or even 'costexpr const char* const'? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@78 PS4, Line 78: const Schema& schema If the schema of the tablet is known beforehand, why to pass it around and search for column indices every time? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@126 PS4, Line 126: SchemaToPB(schema, req.mutable_schema() Is it possible to do convert the known schema to PB just once and use it for building write request PB messages? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@139 PS4, Line 139: record_types.push_back(TRANSACTION); : record_types.push_back(PARTICIPANT); Why is it required? Does the transaction table store other than transaction records? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@222 PS4, Line 222: SchemaBuilder builder; : CHECK_OK(builder.AddKeyColumn(kTxnIdColName, INT64)); : CHECK_OK(builder.AddKeyColumn(kEntryTypeColName, INT8)); : CHECK_OK(builder.AddKeyColumn(kIdentifierColName, STRING)); : CHECK_OK(builder.AddNullableColumn(kUserColName, STRING)); : CHECK_OK(builder.AddColumn(kMetadataColName, STRING)); : return builder.Build(); Why not to make construct this once (e.g., use a static) and then return a constant reference to that? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@293 PS4, Line 293: tablet_replica_->tablet()->schema()->CopyWithoutColumnIds(); Could this be constructed even without the underlying tablet replica, like in TxnStatusTablet::GetSchema()? Or it's intentional that this is taken directly from the underlying tablet? If the latter is true, then maybe rename this method to reflect the fact somehow (like GetActualTabletSchema(), etc.)? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@319 PS4, Line 319: Corruption Why Status::Corruption? Would Status::Incomplete() be a better option here? http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/transactions.proto File src/kudu/transactions/transactions.proto: http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/transactions.proto@28 PS4, Line 28: status Where do we store the rest of metadata pertained to a transaction? Like 'start' timestamp, etc.? -- To view, visit http://gerrit.cloudera.org:8080/16043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c Gerrit-Change-Number: 16043 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 11 Jun 2020 17:54:29 +0000 Gerrit-HasComments: Yes
