Todd Lipcon has posted comments on this change. Change subject: KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc ......................................................................
Patch Set 5: (13 comments) http://gerrit.cloudera.org:8080/#/c/5057/5/src/kudu/tablet/mvcc-test.cc File src/kudu/tablet/mvcc-test.cc: PS5, Line 217: TestOfflineTransactions can you update the terminology and the comment here? we don't use the word 'offline' in MVCC anymore so this test is now confusing Line 228: // and committing this transaction "offline" this same Line 546: // coalesce to the latest timestamp, for offline transactions. same (and elsewhere -- grep for 'offline' in this test) 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 InitTransactionUnlocked returns false due to timestamp being lower than no_new_transactions_at_or_before_. I also noticed this comment in InitTransactionUnlocked: // Ensure that we didn't mark the given timestamp as "safe" in between // acquiring the time and taking the lock. This allows us to acquire timestamps // outside of the MVCC lock. This comment was added by b6f141f37c917ff1bc2607dc6808a680ca2b2c7d which has some interesting notes in its commit message about lock contention in the MVCC stuff. Might be worth doing a before/after sanity check of perf using the benchmark invocations mentioned in that message. Additionally, I think the comment above probably needs to be removed or updated, since I don't entirely understand it anymore. PS5, Line 55: weird indent 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 'subset' relationship is actually false. Here you are still only updating the 'clean' time up to the 'safe' time if I understand correctly, and never above. PS5, Line 182: no_new_transactions_at_or_before_ should we rename this member variable to 'safe_time_'? 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 PS5, Line 203: without advancing the safe time this probably needs to be updated, right? PS5, Line 211: // Used for bootstrap and delayed processing in FOLLOWERS/LEARNERS. this is used in all cases now Line 218: void AdjustSafeTime(Timestamp safe_time); can you add a note about when this is expected to be called on leaders vs followers etc? Line 381: explicit ScopedTransaction(MvccManager* manager, Timestamp timestamp); can remove 'explicit' here Line 411: Timestamp timestamp_; can be const now -- 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
