Will Berkeley has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs ......................................................................
Patch Set 2: (16 comments) http://gerrit.cloudera.org:8080/#/c/6809/2//COMMIT_MSG Commit Message: PS2, Line 11: pending leader > not sure what the pending leader is. Configs themselves don't have leaders. You're right. I'm getting rid of it. http://gerrit.cloudera.org:8080/#/c/6809/2/src/kudu/consensus/consensus_meta.cc File src/kudu/consensus/consensus_meta.cc: PS2, Line 181: committed_cstate > rename to cstate Done http://gerrit.cloudera.org:8080/#/c/6809/2/src/kudu/consensus/metadata.proto File src/kudu/consensus/metadata.proto: PS2, Line 113: leader_uuid > I think this is a little confusing. Can this just be the current leader, an Done. I had the master wipe the leader_uuid that's not in the committed config from the reported cstate when it processes a tablet report. It seemed like that should be the only change needed to keep the old behavior. PS2, Line 117: config > +1 Done PS2, Line 117: config > Rename to committed_config; Protobuf allows you to rename things as long as Done PS2, Line 121: pending_leader_uuid > I'm not sure why this needs to be specified separately. I think we should r Done PS2, Line 121: pending_leader_uuid > yea, I am a bit confused about this -- a leader is associated with a term, Done http://gerrit.cloudera.org:8080/#/c/6809/2/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: PS2, Line 109: string leader_uuid = cstate.has_pending_config() ? : cstate.pending_leader_uuid() : cstate.leader_uuid(); > this part is still odd to me Done PS2, Line 111: RaftConfigPB config > can be const ref Done PS2, Line 190: UNCOMMITTED_QUORUM > we should really rename these constants to COMMITTED_CONFIG and PENDING_CON Done http://gerrit.cloudera.org:8080/#/c/6809/2/src/kudu/integration-tests/cluster_itest_util.cc File src/kudu/integration-tests/cluster_itest_util.cc: Line 48: using client::KuduClient; > warning: using decl 'KuduClient' is unused [misc-unused-using-decls] Done Line 484: // NB: FIX: wants active, not committed > yea, I think this should be fixed before committing? or at least verified t My bad. Done. http://gerrit.cloudera.org:8080/#/c/6809/2/src/kudu/tserver/tablet_copy_source_session.cc File src/kudu/tserver/tablet_copy_source_session.cc: PS2, Line 131: initial_committed_cstate_ > is there anything we have to worry about with a copy going on at the same t Done PS2, Line 131: initial_committed_cstate_ > Rename to initial_cstate_ Done PS2, Line 131: initial_committed_cstate_ > It's fine because it just uses the committed consensus state from the copy Done http://gerrit.cloudera.org:8080/#/c/6809/2/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: PS2, Line 951: committed_consensus_state > Rename to consensus_state 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: 2 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
