Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage ...................................................................... Patch Set 5: (18 comments) 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 int Done http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@19 PS4, Line 19: > nit: use <cstdint> instead? Done http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@31 PS4, Line 31: > nit: drop this extra line? Done http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@41 PS4, Line 41: > Is this for the typedef or for the class TransactionVisitor? I guess it's Done http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@45 PS4, Line 45: > Do you think it's worth describing the semantics of various return statuses Actually this will never fail. Updated the type. http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@58 PS4, Line 58: > Would the value for this column be NULL for PARTICIPANT records? If so, ma Good point -- I'd originally wanted to keep the 'metadata' record limited to the state that we expect to change per entry. That way any rewritten data can be limited to the state that has actually changed. That said, it is a bit of a premature optimization. Probably better to keep it simple for now. http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@71 PS4, Line 71: > I'm curious what 'physically thread-safe' means. Does it imply extra threa Yeah, I suppose I just meant "thread-safe", though the primary caller of this (i.e. the TxnStatusManager) will need to enforce consistency on its end. http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@90 PS4, Line 90: > What is the relation between the schema of the tablet replica coming in her I was trying to be a bit conservative w.rt. the schema, in case in the future we decided to change it, at least we could still operate the table. OTOH, I suppose it's straightforward enough to simply crash or fail the tablet in case we change schemas, and require some tooling to migrate this table. http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@91 PS4, Line 91: > nit: is it worth adding DCHECK(tablet_replica_) in the constructor? Done http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.cc File src/kudu/transactions/status_tablet.cc: http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.cc@193 PS2, Line 193: > std::make_pair shouldn't be needed with emplace_back Done 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: > nit: use <cstddef> ? Done 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 'co Done http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@126 PS4, Line 126: > Is it possible to do convert the known schema to PB just once and use it fo Done http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@139 PS4, Line 139: : > Why is it required? Does the transaction table store other than transactio This is defensive, considering we've added entry types to the system catalog table. I'll add a comment. http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@222 PS4, Line 222: : : : : : : > Why not to make construct this once (e.g., use a static) and then return a Done http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@293 PS4, Line 293: > Could this be constructed even without the underlying tablet replica, like I was at first trying to be mindful of the possibility of schema changes, but I think that's a bit of a premature optimization. http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@319 PS4, Line 319: > Why Status::Corruption? Would Status::Incomplete() be a better option here Done 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: -bones > Where do we store the rest of metadata pertained to a transaction? Like 's I left extraneous fields out and went with a simple message for now; we'll add those fields as they become necessary to implement transactions further. -- 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: 5 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: Sat, 13 Jun 2020 05:06:06 +0000 Gerrit-HasComments: Yes
