David Ribeiro Alves has posted comments on this change. Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc ......................................................................
Patch Set 5: (25 comments) http://gerrit.cloudera.org:8080/#/c/5057/5//COMMIT_MSG Commit Message: PS5, Line 9: This > nit: This --> This patch Done PS5, Line 9: that > nit: that --> the or that --> those or, may be, drop 'that'? Done PS5, Line 14: This patch takes this the rest of the way by > nit: may be 'This patch completes that by ...' Done 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? simplified, I don't think it needs it now 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 Done 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 Done 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 Done 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 Done 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: wh this test is not new nor is it changed in any way funcionally. would prefer to avoid fixing these random backlog things in what are mostly mechanical changes. PS5, Line 105: // Start timestamp 1, timestamp 2 > What does 'Start timestamp' mean? May be, update this comment to clarify t removed PS5, Line 164: mgr.AdjustSafeTime(t3); > Why did it appear in this revision of the test? Does this mean the previou this patch is about removing automatic safe time adjustment apis from mvcc. since we don't have the automatic apis anymore we have to do it "manually" (this would be called within CommitTransaction above). Added a comment to that regard though honestly don't know whether it conveyed much more information than the code statement. PS5, Line 217: TestOfflineTransactions > can you update the terminology and the comment here? we don't use the word Done Line 228: // and committing this transaction "offline" this > same Done Line 546: // coalesce to the latest timestamp, for offline transactions. > same (and elsewhere -- grep for 'offline' in this test) Done PS5, Line 552: CHECK_OK > Why not CHECK_OK(), not ASSERT_OK() ? Done http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/mvcc.cc File src/kudu/tablet/mvcc.cc: PS5, Line 54: There is already a transaction with timestamp: $0 in flight > this error string isn't necessarily accurate in the case that InitTransacti yeah this race can't happen anymore because ts assignment and txn start are done under a lock now, for test. did a sanity check. there was a slowdown of about 10% on mt-tablet-test, but this is not the actual tserver write patch. ran full_stack-insert-scan-test there are there is actually a speedup of 9%. PS5, Line 55: > weird indent Done PS5, Line 125: // If this transaction was the earliest in-flight, we might have to adjust : // the "clean" timestamp. : AdjustCleanTime(); > ah, so my comment in the other review about clean/safe time not having a 's yeah, safe/clean time behave as before. As I mentioned on the other patch (funnily or not :)) until now this patch series doesn't actually change any behavior. We were advancing "safe" time before apply. meaning that the "automatic' advancement on commit for leader txns would only really either move the "clean" time or nothing at all, which is what still happens now. PS5, Line 182: no_new_transactions_at_or_before_ > should we rename this member variable to 'safe_time_'? Done http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/mvcc.h File src/kudu/tablet/mvcc.h: PS5, Line 168: // When a transaction is started, a timestamp is assigned. The manager will : // never assign a timestamp if there is already another transaction with : // the same timestamp in flight or previously committed. > needs to be updated Done PS5, Line 203: without advancing the safe time > this probably needs to be updated, right? Done PS5, Line 211: // Used for bootstrap and delayed processing in FOLLOWERS/LEARNERS. > this is used in all cases now Done Line 218: void AdjustSafeTime(Timestamp safe_time); > can you add a note about when this is expected to be called on leaders vs f Part of the point of this patch series is to make mvcc oblivious to leaders/followers so would rather not mention them specifically, just the general constraints around 'safe_time'. Pointed to the WIP doc (that is not merged) on the class header as it will be the place where these concepts live. Afraid I will forget if I postpone linking for later and doesn't seem jira worthy. Line 381: explicit ScopedTransaction(MvccManager* manager, Timestamp timestamp); > can remove 'explicit' here Done Line 411: Timestamp timestamp_; > can be const now Done -- 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: David Ribeiro Alves <dral...@apache.org> 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
