Alexey Serbin 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 6: (17 comments) Looks good overall, just nits and clarification questions. http://gerrit.cloudera.org:8080/#/c/16510/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16510/6//COMMIT_MSG@64 PS6, Line 64: snapshots nit: timestamps ? http://gerrit.cloudera.org:8080/#/c/16510/6//COMMIT_MSG@87 PS6, Line 87: This nit: This patch ? 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)", *txn_id_) : "") : << ": row " << schema_.DebugRow(row) nit: does it make sense to output commit timestamp as well if it's present? http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/memrowset.cc@539 PS6, Line 539: committed nit: I guess this would be 'applied' in case if it's non-transactional op. Maybe, update this to reflect existence of both the 'applied' and the 'committed' semantics? 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 { Looking at the set of added test scenarios I'm curious whether we have enough coverage for both types of MvccSnapshot (i.e. SnapshotType::{kLatest,kTimestamp}). If you think this set is good enough, could you please add small blurbs emphasizing the specifics of the snapshot types involved in particular tests scenarios (if this makes any sense, of course)? 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(); nit: just wanted to clarity how it would like in real world scenarios. I guess ScopedOp is designed to implicitly call Abort() in the destructor unless FinishApplying() or Abort() was called. From this perspective and for illustrative purposes, should op.FinishApplying() be called before txn_meta->set_commit_timestamp(commit_ts) ? Otherwise, how to guarantee that an would not become aborted if there is some transaction-related activity in between op.StartApplying() and op.FinishApplying(), and that activity might lead to a failed/aborted transaction? 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 on an optional 'txn_meta'. Would it make sense to add the optional txn_meta to ScopedOp() and call appropriate methods on an op and txn_meta together? 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 aborts after ScopedOp::StartApplying and before ScopedOp::FinishApplying or there is a reason why any code cannot lead to situations like that? Yes, I saw a class-wide comment for MvccManager class, but I'm not sure how those 'right' paths for an op are enforced. http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc-test.cc@896 PS6, Line 896: to nit: with ? http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc-test.cc@899 PS6, Line 899: MvccManager mgr nit: move this closer to the site of the usage? 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)) ? 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 nonapplied nit: does it make sense to update the comment w.r.t. newly introduced 'committed' semantics? http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc.h@115 PS6, Line 115: *commit_ts < all_applied_before_ What about comparing 'non-watermark' timestamps stored in the 'applied_timestamps_' container? Could you add a blurb explaining why it's enough to compare 'commit_ts' with 'all_applied_before_' and spare the comparison of the commit timestamp with elements in that container? http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc.h@428 PS6, Line 428: MvccSnapshot cur_snap_; nit: while you are here, could you add a line of documentation for this member field? I guess it would be nice to know what sort of state this field stores w.r.t. the methods which MvccManager has in its API. 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 InFlightOpsMap::value_type entry nit: maybe, use 'auto' here for (const auto& entry : ops_in_flight_) { ... } http://gerrit.cloudera.org:8080/#/c/16510/6/src/kudu/tablet/mvcc.cc@418 PS6, Line 418: const InFlightOpsMap::value_type entry nit: const auto& entry ? 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 <atomic> nit: is this actually needed here? -- 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: 6 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 20 Oct 2020 05:51:18 +0000 Gerrit-HasComments: Yes
