Alexey Serbin has posted comments on this change. Change subject: KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc ......................................................................
Patch Set 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/5057/5//COMMIT_MSG Commit Message: PS5, Line 9: that nit: that --> the or that --> those or, may be, drop 'that'? PS5, Line 9: This nit: This --> This patch PS5, Line 14: This patch takes this the rest of the way by nit: may be 'This patch completes that by ...' PS5, Line 10: Previous patches : had taken care of making sure these were unused in regular : TabletServer operations or in tests that use LocalTabletWriter. : : This patch takes this the rest of the way by removing the APIs : from Mvcc completely and changing tests that use it directly : to obtain the timestamps from a clock. nit: may be, separate this into its own paragraph? http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/compaction-test.cc File src/kudu/tablet/compaction-test.cc: PS5, Line 174: Timestamp t = clock_->Now(); : ScopedTransaction tx(&mvcc_, t Here and elsewhere: consider removing the temporary variable ScopedTransaction tx(&mvcc_, clock_->Now()); http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/deltamemstore-test.cc File src/kudu/tablet/deltamemstore-test.cc: PS5, Line 82: Timestamp t = clock_->Now(); : ScopedTransaction tx(&mvcc_, t); nit: consider instead ScopedTransaction tx(&mvcc_, clock_->Now()); http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/diskrowset-test.cc File src/kudu/tablet/diskrowset-test.cc: PS5, Line 335: Timestamp t = clock_->Now(); : ScopedTransaction tx(&mvcc_, t); nit: consider ScopedTransaction tx(&mvcc_, clock_->Now()); http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/memrowset-test.cc File src/kudu/tablet/memrowset-test.cc: PS5, Line 110: Timestamp t = clock_->Now(); : ScopedTransaction tx(&mvcc_, t); Here and elsewhere, consider ScopedTransaction tx(&mvcc_, clock_->Now()); http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/mvcc-test.cc File src/kudu/tablet/mvcc-test.cc: PS5, Line 101: TEST_F(MvccTest, TestMvccMultipleInFlight) It would be nice to have a concise description for the idea of the test: what exactly it verifies and how. PS5, Line 105: // Start timestamp 1, timestamp 2 What does 'Start timestamp' mean? May be, update this comment to clarify that or remove the comment at all -- if it's just starting transaction at timestamp, it's clear from the code itself. PS5, Line 164: mgr.AdjustSafeTime(t3); Why did it appear in this revision of the test? Does this mean the previous revisions did something different? Overall, it would be nice to have a comment explaining why the safe time has been adjusted. PS5, Line 552: CHECK_OK Why not CHECK_OK(), not ASSERT_OK() ? -- To view, visit http://gerrit.cloudera.org:8080/5057 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
