[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Will Berkeley has uploaded a new change for review. http://gerrit.cloudera.org:8080/6809 Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config and the pending leader, if the pending config has a leader. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. This change would be backwards-incompatible (the master receives a ConsensusStatePB in a tablet report) but because the new semantics are that the old fields on the ConsensusStatePB hold the committed config, and the master only ever looked at the committed config, it's ok. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 24 files changed, 78 insertions(+), 122 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/1 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#2). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config and the pending leader, if the pending config has a leader. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. This change would be backwards-incompatible (the master receives a ConsensusStatePB in a tablet report) but because the new semantics are that the old fields on the ConsensusStatePB hold the committed config, and the master only ever looked at the committed config, it's ok. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 24 files changed, 80 insertions(+), 126 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/2 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Will Berkeley has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/6809/1/src/kudu/integration-tests/cluster_itest_util.cc File src/kudu/integration-tests/cluster_itest_util.cc: Line 51: using client::KuduTable; > warning: using decl 'KuduTable' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/6809/1/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: Line 43: using consensus::OpId; > warning: using decl 'OpId' is unused [misc-unused-using-decls] Rebased and this became used Line 44: using consensus::RECEIVED_OPID; > warning: using decl 'RECEIVED_OPID' is unused [misc-unused-using-decls] 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Mike Percy has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 2: (5 comments) 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, and if it's not part of the committed config then the master will have to figure that out? PS2, Line 117: config Rename to committed_config; Protobuf allows you to rename things as long as the position doesn't change. PS2, Line 121: pending_leader_uuid I'm not sure why this needs to be specified separately. I think we should remove this field. 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_ Rename to initial_cstate_ 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 -- 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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Todd Lipcon has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 2: (8 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. Does this mean "the node who I voted for in the current term"? http://gerrit.cloudera.org:8080/#/c/6809/2/src/kudu/consensus/metadata.proto File src/kudu/consensus/metadata.proto: PS2, Line 117: config > Rename to committed_config; Protobuf allows you to rename things as long as +1 PS2, Line 121: pending_leader_uuid > I'm not sure why this needs to be specified separately. I think we should r yea, I am a bit confused about this -- a leader is associated with a term, rather than a configuration, so there isn't really any concept of a 'pending' leader (see my comment elsewhere) 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 PS2, Line 111: RaftConfigPB config can be const ref PS2, Line 190: UNCOMMITTED_QUORUM we should really rename these constants to COMMITTED_CONFIG and PENDING_CONFIG 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 484: // NB: FIX: wants active, not committed yea, I think this should be fixed before committing? or at least verified to not be an issue (and removed) 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_ > Rename to initial_cstate_ is there anything we have to worry about with a copy going on at the same time as a config change? I dont quite remember what we do with this initial_cstate_ variable but making me nervous that we might now be copying one more bit of state than we used to, or somesuch? -- 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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Mike Percy has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 2: (2 comments) 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 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 It's fine because it just uses the committed consensus state from the copy source (at the time the tablet copy session was initiated) and sends it to the tablet copy target replica so it can have something to start with. Then the target replays the WAL so it will get whatever it needs from that. The code that uses it is in TabletCopyClient::WriteConsensusMeta() which calls calls cmeta_->MergeCommittedConsensusStatePB(*remote_committed_cstate_) to update its committed config and term from the remote[ We should also rename remote_committed_cstate_ to remote_cstate_ in TabletCopyClient. We may want to just egrep the code base for "committed.*cstate" and rename those variables to remove the committed part. -- 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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#3). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config and the pending leader, if the pending config has a leader. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. This change would be backwards-incompatible (the master receives a ConsensusStatePB in a tablet report) but because the new semantics are that the old fields on the ConsensusStatePB hold the committed config, and the master only ever looked at the committed config, it's ok. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 203 insertions(+), 263 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/3 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#4). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config and the pending leader, if the pending config has a leader. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. This change would be backwards-incompatible (the master receives a ConsensusStatePB in a tablet report) but because the new semantics are that the old fields on the ConsensusStatePB hold the committed config, and the master only ever looked at the committed config, it's ok. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 204 insertions(+), 263 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/4 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#5). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 212 insertions(+), 265 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/5 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#6). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 215 insertions(+), 268 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/6 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Will Berkeley has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/6809/5/src/kudu/integration-tests/ts_tablet_manager-itest.cc File src/kudu/integration-tests/ts_tablet_manager-itest.cc: Line 184: ReportedTabletPB reported_tablet = report.updated_tablets(0); > warning: the variable 'reported_tablet' is copy-constructed from a const re Done http://gerrit.cloudera.org:8080/#/c/6809/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 3797: // TODO: GetConsensusRole() iterates over all of the peers, making this an > warning: missing username/bug in TODO [google-readability-todo] git blamed on adar http://gerrit.cloudera.org:8080/#/c/6809/5/src/kudu/tserver/ts_tablet_manager-test.cc File src/kudu/tserver/ts_tablet_manager-test.cc: Line 159: for (ReportedTabletPB reported_tablet : report.updated_tablets()) { > warning: loop variable is copied but only used as const reference; consider 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: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Mike Percy 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 pending configuration (or both). What the master does with that information isn't part of the contract of this data structure. PS6, Line 113: 5 4 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 PS6, Line 199: nit: funny extra indentation Line 219: string DiffConsensusStates(const ConsensusStatePB& old_state, Looks like this needs to detect differences in pending configs now too. 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 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 the comment. 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 never persist a pending config. 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 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 -- 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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#7). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 331 insertions(+), 301 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/7 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#8). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 333 insertions(+), 301 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/8 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Todd Lipcon has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/6809/8/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: PS8, Line 197: return Status::IllegalState( : Substitute("Leader with UUID $0 is not a VOTER in the committed or pending config! " :"Consensus state: $1", hrm, isn't this possible? eg when catching up a node that is more than one config change behind, you might have a leader which isn't in its committed or pending config yet? Line 219: namespace { nit: don't indent inside namespace Line 221: typedef map> PeerInfoMap; Add a comment about the structure of this map (key and value contents?) PS8, Line 332: gained a gained/lost is a bit strange terminology here, since it sounds sort of like there's a list. Maybe just "changed pending config" or "now has pending config" or somesuch -- 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: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Mike Percy has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 8: (4 comments) I only took a quick look at rev 8; there are a bunch of review comments missed in rev 8 marked as done in rev 6 so I think a rebase got messed up or something. 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. > Done Not done yet in rev 8 PS6, Line 113: 5 > Done Not done in rev 8 http://gerrit.cloudera.org:8080/#/c/6809/8/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: PS8, Line 197: return Status::IllegalState( : Substitute("Leader with UUID $0 is not a VOTER in the committed or pending config! " :"Consensus state: $1", > hrm, isn't this possible? eg when catching up a node that is more than one I actually don't think it's possible to hit this in the current production code because we run this right after the consensus meta is loaded from disk. However I talked to Will about this offline and suggested he define a different function to call from sys_catalog.cc and then we can leave this one for use by tests. Line 304: Substitute("term changed from $0 to $1", nit: 4 line indentation for a continued line per https://google.github.io/styleguide/cppguide.html#Function_Calls -- 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: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#9). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 331 insertions(+), 300 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/9 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Will Berkeley has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 6: (7 comments) Also hit all the comments that somehow got missed from PS6. 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. > Not done yet in rev 8 Sorry. Done. PS6, Line 113: 5 > Not done in rev 8 Sorry. Done. http://gerrit.cloudera.org:8080/#/c/6809/8/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: PS8, Line 197: return Status::IllegalState( : Substitute("Leader with UUID $0 is not a VOTER in the committed or pending config! " :"Consensus state: $1", > I actually don't think it's possible to hit this in the current production Done. Omitting redundant checks for required fields and the leader check, the only thing that remains is to check that the committed_config is valid and that we're not storing a pending_config. Line 219: string DiffConsensusStates(const ConsensusStatePB& old_state, > nit: don't indent inside namespace Done Line 221: bool leader_changed = old_state.leader_uuid() != new_state.leader_uuid(); > Add a comment about the structure of this map (key and value contents?) Done Line 304: if (SecureShortDebugString(old_state) == SecureShortDebugString(new_state)) { > nit: 4 line indentation for a continued line per https://google.github.io/s Done PS8, Line 332: > gained/lost is a bit strange terminology here, since it sounds sort of like 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 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#10). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 331 insertions(+), 302 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/10 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Todd Lipcon has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 10: Code-Review+1 -- 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: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Mike Percy has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 10: Code-Review+1 (1 comment) Looks good to me, just one nit. http://gerrit.cloudera.org:8080/#/c/6809/10/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: PS10, Line 199: nit: sorry to nitpick so hard on this, can we just align this opening quote with the opening quote on the above line? The GSG says it best: https://google.github.io/styleguide/cppguide.html#Function_Calls If the arguments do not all fit on one line, they should be broken up onto multiple lines, with each subsequent line aligned with the first argument. Do not add spaces after the open paren or before the close paren: bool result = DoSomething(averyveryveryverylongargument1, argument2, argument3); -- 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: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Mike Percy, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#11). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 331 insertions(+), 300 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/11 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Will Berkeley has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/6809/10/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: PS10, Line 199: > nit: sorry to nitpick so hard on this, can we just align this opening quote It's technically a continuation of the first argument, not a second argument, but done. It's the IDE auto-formatting it that way, so I'll have to review my settings. -- 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: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Will Berkeley has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 11: > Build Failed > > http://104.196.14.100/job/kudu-gerrit/7954/ : FAILURE I think this is due to the proto3 changes. Rebasing. -- 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: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Mike Percy, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#12). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 331 insertions(+), 301 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/12 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Mike Percy, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#13). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 333 insertions(+), 304 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/13 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Todd Lipcon has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 13: (2 comments) http://gerrit.cloudera.org:8080/#/c/6809/13/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: Line 195: && cstate .has_pending_config() weird space? Line 375: also weird spacing -- 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: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Will Berkeley has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 13: (2 comments) http://gerrit.cloudera.org:8080/#/c/6809/13/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: Line 195: && cstate .has_pending_config() > weird space? Done Line 375: > also weird spacing 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: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Mike Percy, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#14). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 333 insertions(+), 306 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/14 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Mike Percy has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 14: Code-Review+2 -- 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: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Todd Lipcon has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 14: Code-Review+2 -- 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: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Hello Mike Percy, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6809 to look at the new patch set (#15). Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 334 insertions(+), 306 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/6809/15 -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Todd Lipcon has posted comments on this change. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Patch Set 15: Code-Review+2 -- 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: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Todd Lipcon has submitted this change and it was merged. Change subject: Refactor ConsensusStatePB to hold committed and pending configs .. Refactor ConsensusStatePB to hold committed and pending configs This patch refactors ConsensusStatePB so it contains both the committed config and, if there is a pending config, the pending config. Previously, consumers of the consensus state would ask for either the committed or active (= pending if there is a pending config, else the committed config) config. With this change, the committed and active configs are always included. The addition of pending info to the ConsensusStatePB will be used by ksck in a follow-up patch. Additionally, the master may use this info in the future to be more aware of the health of tablets. Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Reviewed-on: http://gerrit.cloudera.org:8080/6809 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M src/kudu/consensus/consensus.h M src/kudu/consensus/consensus.proto M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/metadata.proto M src/kudu/consensus/quorum_util-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-path-handlers.cc M src/kudu/master/master.proto M src/kudu/master/sys_catalog.cc M src/kudu/tools/kudu-admin-test.cc M src/kudu/tserver/tablet_copy.proto M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 34 files changed, 334 insertions(+), 306 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I4bc4bdd9752fc29a7ce2cefcdc95c4366f5353af Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley