Will Berkeley has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs ......................................................................
Patch Set 6: (10 comments) http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/consensus/metadata.proto File src/kudu/consensus/metadata.proto: PS6, Line 103: There is a corner case in Raft where a node may be elected leader of a : // pending (uncommitted) configuration. In such a case, the master will : // detect that the node is not a member of the committed configuration. > I think you can just say: The leader may be a part of the committed or the Done PS6, Line 113: 5 > 4 Done http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: PS6, Line 195: > nit: stray space here Done PS6, Line 199: > nit: funny extra indentation Done Line 219: string DiffConsensusStates(const ConsensusStatePB& old_state, > Looks like this needs to detect differences in pending configs now too. Done http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS6, Line 2235: > nit: indent at least an extra 4 spaces for a line wrap Done PS6, Line 2362: Previously, this field was omitted from the : // consensus state in this situation. > I don't think this helps clarify this code, consider removing this part of Done http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: Line 134: RETURN_NOT_OK(consensus::VerifyConsensusState(cstate)); > Add something like CHECK(!cstate.has_pending_config()) since we should neve Done http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/tserver/tablet_copy.proto File src/kudu/tserver/tablet_copy.proto: PS6, Line 116: committed > remove this word Done http://gerrit.cloudera.org:8080/#/c/6809/6/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: PS6, Line 951: *reported_tablet->mutable_consensus_state() = : consensus->ConsensusState(); > nit: This will fit on one line now Done -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-HasComments: Yes
