You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2017/05/05 21:33:54 UTC
[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 <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
---
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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<string, pair<RaftPeerPB, RaftPeerPB>> 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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] Refactor ConsensusStatePB to hold committed and pending configs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot