Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16510 )
Change subject: KUDU-2612: have MRS iteration account for txn metadata ...................................................................... Patch Set 7: (22 comments) http://gerrit.cloudera.org:8080/#/c/16510/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16510/6//COMMIT_MSG@35 PS6, Line 35: waits f > waits Done http://gerrit.cloudera.org:8080/#/c/16510/6//COMMIT_MSG@64 PS6, Line 64: tamp snap > nit: timestamps ? This is referring to kTimestamp snapshots. http://gerrit.cloudera.org:8080/#/c/16510/6//COMMIT_MSG@87 PS6, Line 87: This > nit: This patch ? Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/memrowset-test.cc File src/kudu/tablet/memrowset-test.cc: http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/memrowset-test.cc@958 PS6, Line 958: how we iterate > Does it make sense to add coverage for CheckRowsBetween() in between latest Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/memrowset-test.cc@999 PS6, Line 999: rt(); > Ditto: what about CheckRowsBetween() in between latest_with_none_applied, Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/memrowset.h@367 PS6, Line 367: he > nit: remove? Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/memrowset.cc@156 PS6, Line 156: << string(txn_id_ ? : Substitute("(txn_id=$0$1)" > nit: does it make sense to output commit timestamp as well if it's present? Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/memrowset.cc@539 PS6, Line 539: > nit: I guess this would be 'applied' in case if it's non-transactional op. Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc-test.cc File src/kudu/tablet/mvcc-test.cc: http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc-test.cc@787 PS6, Line 787: class TransactionMvccTest : public MvccTest { > Ah, I guess the coverage for different types of snapshots is provided by th FWIW, the semantics of kLatest and kTimestamp snapshots are not new -- this patch just formalizes the two distinct usages we already have. http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc-test.cc@799 PS6, Line 799: Timestamp commit_ts = Timestamp(ts.value() + 10); : txn_meta->set_commit_timestamp(commit_ts); : op.FinishApplying(); > how to guarantee that an would not become aborted if there is some > transaction-related activity The transaction state lock prevents concurrent participant ops from running, so while the FINALIZE_COMMIT op is in flight, we cannot call ABORT_TXN. > should op.FinishApplying() be called before > txn_meta->set_commit_timestamp(commit_ts) It is an invariant that the commit timestamp is set before the MVCC op is completed. Take this example: BEGIN_COMMIT for txn 1 - We start a ScopedOp at ts 1000 - All snapshot scans higher than ts 1000 are blocked until the ScopedOp finishes FINALIZE_COMMIT for txn 1 with commit ts 1010 - op.StartApplying() is called to signify the FINALIZE_COMMIT op is going to be applied - Per your suggestion, say we _didn't_ set the commit timestamp right away, but did so later - op.FinishApplying() is called to signify the commit is complete SCAN at ts 1020 - Because the commit ScopedOp is complete, we don't block - But when we iterate through data, the transaction's commit timestamp hasn't been set! We don't know the appropriate commit timestamp to consider! http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc-test.cc@812 PS6, Line 812: txn_meta->set_aborted(); : op.Abort(); > nit: looking at this makes me think that ScopedOp could comprise operations Not a bad thought, but not really. At least, there would be a significant amount of plumbing to get that working, based on how the BEGIN_COMMIT ParticipantOp is written today today. http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc-test.cc@817 PS6, Line 817: AbortBeforeBeginCommit > nit: does it make sense to add coverage for the case when a transaction abo TestIllegalStateTransitionsCrash is an example here. We only ever call StartApplying() and FinishApplying() from the same op driver. In the case of a commit, that's the op driver for the FINALIZE_COMMIT op. Once we call StartApplying(), we _will_ call FinishApplying() -- that is how the code is written today, and there are DCHECKs in place preventing us from deviating from that. One example of this invariant is the CHECK_EQ(old_state, RESERVED) in MvccManager::AbortOp() that ensures that we haven't begun applying the op yet. http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc-test.cc@896 PS6, Line 896: it > nit: with ? Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc-test.cc@899 PS6, Line 899: constexpr const > nit: move this closer to the site of the usage? Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc-test.cc@912 PS6, Line 912: MvccSnapshot > nit: could this be std::move(MvccSnapshot(mgr)) ? Isn't this already an rvalue though? http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc.h File src/kudu/tablet/mvcc.h: http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc.h@55 PS6, Line 55: ops with lower timestamps to : // be applied, and those with higher timestamps to be non-applie > nit: does it make sense to update the comment w.r.t. newly introduced 'comm Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc.h@115 PS6, Line 115: > What about comparing 'non-watermark' timestamps stored in the 'applied_time That container should be empty -- the entire kTimestamp snapshot should be represented by a single timestamp. I'll add a DCHECK for it too. http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc.h@428 PS6, Line 428: void AdvanceEarliestInF > nit: while you are here, could you add a line of documentation for this mem Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc.cc File src/kudu/tablet/mvcc.cc: http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc.cc@393 PS6, Line 393: const auto& entry : ops_in_flight_) { > nit: maybe, use 'auto' here Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc.cc@418 PS6, Line 418: const auto& entry : ops_in_flight_) { > nit: const auto& entry Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/txn_metadata.h File src/kudu/tablet/txn_metadata.h: http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/txn_metadata.h@19 PS6, Line 19: #include <glog/lo > nit: is this actually needed here? Hrm, interesting IWYU didn't call this out. Done http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/txn_metadata.h@84 PS6, Line 84: bool aborted_; > Can you add a comment on which resource the lock is protecting? Done -- To view, visit http://gerrit.cloudera.org:8080/16510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6bb02c6025eea1a327cf9d9ee1f14a38d63ae4ad Gerrit-Change-Number: 16510 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 23 Oct 2020 08:03:09 +0000 Gerrit-HasComments: Yes
