David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/10649 )
Change subject: consensus: separate storage from ConsensusMetadata ...................................................................... Patch Set 4: (5 comments) I guess andrew's point is that if we want to change where/how we store the consensus meta (.e.g if we want to move it to rocksdb or something), with this change we only need to do it in one place, versus having to change consensusmetadata too. http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta-test.cc File src/kudu/consensus/consensus_meta-test.cc: http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta-test.cc@322 PS4, Line 322: const string& peer_uuid = fs_manager_.uuid(); same as mike's comment above. this is a test, I think copying this string (and defending against fs_manager lifecycle bugs) is probably best http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.h File src/kudu/consensus/consensus_meta_manager.h: http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.h@56 PS4, Line 56: Status Init() const; hum, Init() is const? that's a bit weird... http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.h@112 PS4, Line 112: LoadFromDisk do we need to have the "fromdisk" suffix? I thought the point of this change was that we could store the metadata elsewhere, e.g. rocksdb http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.h@117 PS4, Line 117: FlushToDisk same http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.cc File src/kudu/consensus/consensus_meta_manager.cc: http://gerrit.cloudera.org:8080/#/c/10649/4/src/kudu/consensus/consensus_meta_manager.cc@153 PS4, Line 153: const string& cmeta_path = fs_manager_->GetConsensusMetadataPath(tablet_id); : RETURN_NOT_OK_PREPEND(fs_manager_->env()->DeleteFile(cmeta_path), : Substitute("Unable to delete consensus metadata for tablet $0", tablet_id)); : lock_guard<Mutex> l(lock_); : cmeta_cache_.erase(tablet_id); // OK to delete uncached cmeta; ignore the return value. : return Status::OK(); hum, are you sure the change of ordering here doesn't matter? -- To view, visit http://gerrit.cloudera.org:8080/10649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f17123e281789976df1c436fe8116a4d4b48923 Gerrit-Change-Number: 10649 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 25 Jun 2018 17:59:04 +0000 Gerrit-HasComments: Yes
