Todd Lipcon has posted comments on this change. Change subject: KUDU-798 (part 1) - Unify leader/follower mvcc behavior ......................................................................
Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/5055/6//COMMIT_MSG Commit Message: Line 9: Safe time is a timestamp such that all transactions before it are would be great to have the definitions of 'safe' and 'clean' times somewhere in docs/design-docs/ or a block comment in one of the headers. Maybe this information can be moved into such a place and the commit message updated to just refer to the newly-added docs? PS6, Line 17: lantency nit: typo PS6, Line 19: this timestamp you mean the safe time, right? PS6, Line 21: mvcc nit: can you consistetly write it as 'MVCC' for better readability? PS6, Line 30: This patch series aims at separating the two concepts and fixing : safe time advancement: : a I like the general direction here. I think the key thing is that we used to sort of think of safe/clean as having a "subset" relationship and now that's not necessarily the case, right? One concern, having not yet read the patch yet, but just thinking about this: when we construct the 'current MvccSnapshot' for internal-facing purposes like compaction, we need to make sure we don't prematurely use a simple 'TS < <clean timestamp>' snapshot when that snapshot is not also safe, right? Otherwise if we're a follower and a new write arrives with a lower timestamp, it can show up in the middle of a compaction and wreak havoc. How will snapshots chosen for compaction/flush work now, given they also need to be repeatable? http://gerrit.cloudera.org:8080/#/c/5055/6/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: PS6, Line 393: mvcc_tx.reset(new ScopedTransaction(&mvcc_, tx_state->timestamp())); : tx_state->SetMvccTxAndTimestamp(std::move(mvcc_tx)); : } strikes me that this implementation is the same as the 'ForTests' method above in the case that the tx already has the timestamp assigned. Will the above one be phased out? Maybe we can change the above one to CHECK that !tx_state->has_timestamp() and any cases where that check would fire we should be using this new method? http://gerrit.cloudera.org:8080/#/c/5055/6/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: Line 160: void StartTransactionForTests(WriteTransactionState* tx_state); rename this to AssignTimestampAndStartTransactionForTests or somesuch? otherwise it's a bit confusing at call sites. http://gerrit.cloudera.org:8080/#/c/5055/6/src/kudu/tablet/transactions/transaction_driver.cc File src/kudu/tablet/transactions/transaction_driver.cc: Line 264: RETURN_NOT_OK(transaction_->Start()); I'm not 100% sure here, but it strikes me as strange that the order of transaction_->Start() and the ResultTracker registration is getting inverted by this patch. Does that have the potential to cause some issue? PS6, Line 284: // This is a placeholder since in the near future the timestamp will be assigned. : // within consensus. can you add a TODO(dralves) and be more specific about the JIRA that will remove it? Line 286: if (!transaction_->state()->has_timestamp()) { could this be a CHECK? when would we be in this case and not already have a timestamp? http://gerrit.cloudera.org:8080/#/c/5055/6/src/kudu/tablet/transactions/transaction_driver.h File src/kudu/tablet/transactions/transaction_driver.h: Line 193: const scoped_refptr<server::Clock>& clock); I think this should just be a 'scoped_refptr' and then assign it with std::move in the initializer list Line 322: // Temporatily have the clock on the driver so that we can assign timestamps to typo: temporarily also, what do you mean by temporarily here? If you mean that this is transitional and you plan to remove it in a later patch in this series, can you leave a TODO(dralves) with a specific call-out to which JIRA will remove it? Otherwise I think we'll just forget about this forever. -- To view, visit http://gerrit.cloudera.org:8080/5055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3ba7212f9211f585d4bef00e5ccfc24d5eece224 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
