Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16277 )
Change subject: KUDU-2612 p7: add transaction participants to tablets ...................................................................... Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/16277/4/src/kudu/integration-tests/txn_participant-itest.cc File src/kudu/integration-tests/txn_participant-itest.cc: http://gerrit.cloudera.org:8080/#/c/16277/4/src/kudu/integration-tests/txn_participant-itest.cc@84 PS4, Line 84: statuses[i] = nit: maybe, if this assignment assumes that statuses[i].ok() at this moment, add an assertion here (like CHECK/DCHECK)? http://gerrit.cloudera.org:8080/#/c/16277/4/src/kudu/integration-tests/txn_participant-itest.cc@123 PS4, Line 123: NO_FATALS(SetUpTable()); Does it make sense to move it into the SetUp() method? http://gerrit.cloudera.org:8080/#/c/16277/4/src/kudu/integration-tests/txn_participant-itest.cc@200 PS4, Line 200: NO_FATALS(SetUpTable()) Does it make sense to move it into the SetUp() method? http://gerrit.cloudera.org:8080/#/c/16277/4/src/kudu/integration-tests/txn_participant-itest.cc@219 PS4, Line 219: const nit: constexpr ? http://gerrit.cloudera.org:8080/#/c/16277/4/src/kudu/tablet/txn_participant.h File src/kudu/tablet/txn_participant.h: http://gerrit.cloudera.org:8080/#/c/16277/4/src/kudu/tablet/txn_participant.h@135 PS4, Line 135: Status here and below: if there methods cannot return anything except for Status::OK(), maybe make them return void? http://gerrit.cloudera.org:8080/#/c/16277/4/src/kudu/tablet/txn_participant.h@136 PS4, Line 136: DCHECK_OK Is it intended that all Validate... are gone in case of a release build? http://gerrit.cloudera.org:8080/#/c/16277/4/src/kudu/tablet/txn_participant.h@213 PS4, Line 213: void ClearIfInitFailed How do the caller know if it did anything to the requested record? And whether ParticipantOp::Finish() should care about that or not? http://gerrit.cloudera.org:8080/#/c/16277/4/src/kudu/tablet/txn_participant.h@223 PS4, Line 223: inline bool operator==(const TxnParticipant::TxnEntry& lhs, const TxnParticipant::TxnEntry& rhs) { : return lhs.txn_id == rhs.txn_id && : lhs.state == rhs.state && : lhs.commit_timestamp == rhs.commit_timestamp; : } Is this needed? I thought this would be generated automatically by the compiler, no? -- To view, visit http://gerrit.cloudera.org:8080/16277 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39201ce1d911308cd28f3f4790a126e30052f3bf Gerrit-Change-Number: 16277 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 11 Aug 2020 02:45:44 +0000 Gerrit-HasComments: Yes
