Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16515 )
Change subject: KUDU-2612: add txn memrowsets to tablet ...................................................................... Patch Set 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_metadata.cc@816 PS3, Line 816: nit: an extra line? http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_replica-test-base.cc File src/kudu/tablet/tablet_replica-test-base.cc: http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_replica-test-base.cc@98 PS3, Line 98: if (resp.per_row_errors_size() > 0) { : return StatusFromPB(resp.per_row_errors(0).error()); : } Good catch! http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_metadata.h File src/kudu/tablet/txn_metadata.h: http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_metadata.h@43 PS3, Line 43: will ensures nit: 'will ensure' or just 'ensures' http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_metadata.h@44 PS3, Line 44: accesses nit: accessed http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc File src/kudu/tablet/txn_participant-test.cc: http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@173 PS3, Line 173: Status CallParticipantOpCheckResp(int64_t txn_id, ParticipantOpPB::ParticipantOpType op_type, : int64_t ts_val) { : ParticipantResponsePB resp; : RETURN_NOT_OK(CallParticipantOp(tablet_replica_.get(), txn_id, op_type, ts_val, &resp)); : if (resp.has_error()) { : return StatusFromPB(resp.error().status()); : } : return Status::OK(); : } It was a good call to separate this into a function. http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@740 PS3, Line 740: // Once we committed the transaction, we should see the rows. : ASSERT_OK(CallParticipantOpCheckResp(kTxnId, ParticipantOpPB::FINALIZE_COMMIT, : clock()->Now().value())); In addition, does it make sense to verify that no rows are visible if calling ABORT_TXN after BEGIN_COMMIT? http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@844 PS3, Line 844: kAbortedTxnId nit: could be constexpr ? http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@851 PS3, Line 851: s = Write(1, kTxnId); nit for here and below: would it make sense to add a scenario to try to write already existing key (i.e. '0') to be explicit about the exact type of error we are about to receive in that case: Status::AlreadyPresent() or Status::InvalidArgument() ? http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@859 PS3, Line 859: BEGIN_COMMIT Was it supposed to be ABORT_TXN instead? http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@869 PS3, Line 869: Status s = Write(0, kTxnId, RowOperationsPB::UPSERT); Does it make sense to be explicit in this test about the INSERT_IGNORE operation? http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@871 PS3, Line 871: 0 Does it make sense also to verify the behavior if using a not-present key for all those operations? http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@901 PS3, Line 901: ASSERT_EQ(2, rows.size()); In addition, does it make sure that we are about to see the values from the expected transaction to appear? Maybe, in this regard it's enough to use different number of inserted rows for different transactions? http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@917 PS3, Line 917: TestDontReadAbortedInserts Does it make sense to add a hybrid case as well, where a non-transactional UPSERT happens on a row involved in a rolled-back transaction after the transaction rolled back? Also, is it worth to arrange the sequence of transactional updates and non-transactional UPSERT such a way that the UPSERT start after the transaction on the same row begins, but before the transaction aborted? What about sprinkling non-transactional UPDATE_IGNORE before and after aborted transactions and making sure nothing is updated? http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@1263 PS3, Line 1263: TestConcurrentDisjointInsertsTxn Does it make sense to add a hybrid case where in addition to disjoint transactional inserts there concurrent non-transactional UPDATE_IGNORE on the same rows? Or that would make sense only after addressing the note in the commit description about addressing multiple updates into the same rowset? -- To view, visit http://gerrit.cloudera.org:8080/16515 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502 Gerrit-Change-Number: 16515 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 14 Nov 2020 00:12:08 +0000 Gerrit-HasComments: Yes
