David Ribeiro Alves has posted comments on this change. Change subject: KUDU-798 (part 1) - Unify leader/follower mvcc behavior ......................................................................
Patch Set 6: (11 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 somewher this is in progress. I started to dump this info on docs/design-docs/repeatable-reads.md but theres still a some work to make that minimally coherent PS6, Line 17: lantency > nit: typo removed PS6, Line 19: this timestamp > you mean the safe time, right? removed PS6, Line 21: mvcc > nit: can you consistetly write it as 'MVCC' for better readability? removed 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 ab Done 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? othe Done 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 tran I don't think it matters but the reversal was not on purpose. Changed this back. 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 r Done Line 286: if (!transaction_->state()->has_timestamp()) { > could this be a CHECK? when would we be in this case and not already have a good catch. Done 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:: Done Line 322: // Temporatily have the clock on the driver so that we can assign timestamps to > typo: temporarily yeah, thats that I meant. Done -- 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: David Ribeiro Alves <[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
