Mike Percy has posted comments on this change. Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup ......................................................................
Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/7988/7//COMMIT_MSG Commit Message: Line 40: > nit: do you want to mention new tests for CreateOrLoad()? Done http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/consensus/consensus_meta-test.cc File src/kudu/consensus/consensus_meta-test.cc: PS7, Line 121: N > nit: should be lowercased So I looked it up, and apparently you are right. http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/consensus/consensus_meta.h File src/kudu/consensus/consensus_meta.h: PS7, Line 175: is flushed to disk before returning > This is no longer always true. Thanks for the catch. I'll update this. PS7, Line 185: object from disk. > IIUC, Create(NO_FLUSH_ON_CREATE) followed by Load() is an error, right? Is It's not a problem in practice because end-users of this API always go through the ConsensusMetadataManager to acquire a handle to one of these cmeta objects. http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/consensus/consensus_meta_manager-test.cc File src/kudu/consensus/consensus_meta_manager-test.cc: PS7, Line 103: Load > Would using LoadOrCreate here as well be appropriate? Good call. Done. PS7, Line 138: { > nit: this additional scope might be removed, I think. Done http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/consensus/consensus_meta_manager.h File src/kudu/consensus/consensus_meta_manager.h: PS7, Line 62: Returns an error if it cannot be found. > Kind of follows from my other question: IIUC, Create(NO_FLUSH_ON_CREATE) fo Good catch; updated the comment. PS7, Line 71: ConsensusMetadataCreateMode create_mode = : ConsensusMetadataCreateMode::FLUSH_ON_CREATE, > Just making sure I understand this, back-to-back calls to LoadOrCreate(NO_F That's something that is caught by the cache check, so the first call will succeed but the second call will fail. It's tested in ConsensusMetadataManagerTest.TestCreateMultipleUnFlushedCMetas http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: PS7, Line 1088: guarantees that the replica has never voted in a leader election. > I think I understand why this is true (voting --> flushing to disk). Does a It's critical. If it's not true, we lose our Raft safety guarantees and split-brain becomes possible because the same replica can vote differently multiple times in the same term. In practice, if it happened that split brain scenario would probably be rare, but Murphy's Law and all that. Todd and I discussed this on Slack and in person and we decided that there's only so much one can do... so we will essentially consider someone deleting critical files out from under us a type of byzantine failure, which means it's undefined behavior as far as Raft is concerned. It's very hard to do something reasonable to protect against stuff like that. -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> 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
