Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16515 )
Change subject: KUDU-2612: add txn memrowsets to tablet ...................................................................... Patch Set 4: (22 comments) http://gerrit.cloudera.org:8080/#/c/16515/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16515/3//COMMIT_MSG@39 PS3, Line 39: to > to be Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/integration-tests/txn_participant-itest.cc File src/kudu/integration-tests/txn_participant-itest.cc: http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/integration-tests/txn_participant-itest.cc@515 PS3, Line 515: write_threads.emplace_back([&, r, cur_row] { > warning: 'emplace_back' is called inside a loop; consider pre-allocating th Ack http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.h@388 PS3, Line 388: txn_id_ ? st > nit: is this really necessary to construct this extra string? Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.cc@715 PS3, Line 715: > nit: spacing? Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/ops/write_op.h File src/kudu/tablet/ops/write_op.h: http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/ops/write_op.h@178 PS3, Line 178: // Acquires the lock on the given transaction, setting 'txn_' and > nit: does it make sense to add a short doc comment for this method? Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/ops/write_op.h@285 PS3, Line 285: > nit: add a short doc comment (this seems to be the only field without one Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet.cc@1439 PS3, Line 1439: CHECK(s == kOpen || s == kBootstrapping); > warning: The left operand of '==' is a garbage value [clang-analyzer-core.U Ack 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: void TabletMetadata::AddTxnMetadata(int64_t txn_id, unique_ptr<MinLogIndexAnchorer> log_anchor) { > nit: an extra line? Done 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! Ack 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 ensure > nit: 'will ensure' or just 'ensures' Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_metadata.h@44 PS3, Line 44: calls th > nit: accessed Done 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: if (resp.has_error()) { : return StatusFromPB(resp.error().status()); : } : return Status::OK(); : } : : // Writes an op to the WAL, rolls over onto a new WAL segment, and flushes : // the MRS, leaving us with a new WAL segment that should be GC-able unless : / > It was a good call to separate this into a function. Ack http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@740 PS3, Line 740: clock()->Now().value())); : ASSERT_OK(IterateToStrings(&rows)); : ASSERT_EQ(0, rows.size()); > In addition, does it make sense to verify that no rows are visible if calli Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@844 PS3, Line 844: > nit: could be constexpr ? Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@851 PS3, Line 851: ASSERT_OK(CallPartici > nit for here and below: would it make sense to add a scenario to try to wri Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@859 PS3, Line 859: > Was it supposed to be ABORT_TXN instead? Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@869 PS3, Line 869: ASSERT_OK(Write(2, kAbortedTxnId)); > Does it make sense to be explicit in this test about the INSERT_IGNORE oper I'm not sure what this is referring to. Perhaps the implicit INSERT op in Write() with no op argument? If so, I just removed it, since it isn't required to test unsupported ops. http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@871 PS3, Line 871: 2 > Does it make sense also to verify the behavior if using a not-present key f I just removed the initial write, since it was not necessary. http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@901 PS3, Line 901: ASSERT_OK(CallParticipantO > In addition, does it make sure that we are about to see the values from the Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@917 PS3, Line 917: > Does it make sense to add a hybrid case as well, where a non-transactional Done I'm not keen on interleaving updated to the same row, since such interleaving should be disallowed by locking, which hasn't yet been implemented. Done http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@1263 PS3, Line 1263: ()->metrics()->mrs_lookups->valu > Does it make sense to add a hybrid case where in addition to disjoint trans I'm holding off on all testing of concurrent mutations of the same rows, since such access patterns should be prevented via locking. http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant.cc File src/kudu/tablet/txn_participant.cc: http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant.cc@60 PS3, Line 60: *txn_lock = std::m > nit: is there any particular reason to prefer swap() to std::move() notatio Done -- 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: 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-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 18 Nov 2020 07:12:24 +0000 Gerrit-HasComments: Yes
