You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org> on 2020/08/11 00:43:28 UTC
[kudu-CR] WIP [master]: Initiate change config request for adding master
Bankim Bhavsar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16321
Change subject: WIP [master]: Initiate change config request for adding master
......................................................................
WIP [master]: Initiate change config request for adding master
Change includes:
- RPC changes to add a master
- RPC changes to report back member type(VOTER/NON_VOTER) of masters
- Initiates ChangeConfig request to add/remove master
- Happy path test case to add a master with empty sys catalog
- Hidden feature flag "--master_support_change_config" off by default.
TODO:
- Test cases for failure scenarios
Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
---
M src/kudu/common/wire_protocol.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.h
14 files changed, 413 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/16321/1
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16321
to look at the new patch set (#6).
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
[master] KUDU-2181 Raft ChangeConfig request to add a master
This change:
- Adds hidden feature flag "--master_support_change_config" off by
default.
- RPC changes to add a master that initiates Raft config change and
responds asynchronously.
- RPC changes to report back member type(VOTER/NON_VOTER) of masters
- Removes the cached master_addresses in catalog manager as it's no
longer static.
- Updates and adds comments in MasterOptions such that it's used to
fetch master addresses only during master init time as masters can
be added/removed dynamically with this change.
- Updates ListMasters() to look at local Raft config instead of
MasterOptions as the masters can be added/removed dynamically.
If the new master can be caught up from the WAL then the master
gets promoted to VOTER else it remains as NON_VOTER without master
tablet copying support.
Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
---
M src/kudu/common/wire_protocol.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/tool_action_master.cc
20 files changed, 965 insertions(+), 55 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/16321/6
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 6
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [master] KUDU-2181 Raft change config request for adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16321
to look at the new patch set (#3).
Change subject: [master] KUDU-2181 Raft change config request for adding a master
......................................................................
[master] KUDU-2181 Raft change config request for adding a master
Change includes:
- Hidden feature flag "--master_support_change_config" off by default
- RPC changes to add a master that initiates Raft config change and
waits for the completion
- RPC changes to report back member type(VOTER/NON_VOTER) of masters
- Removes the cached master_addresses in catalog manager as it's no longer
static.
- Updates the master_addresses in MasterOptions on successful update to
master Raft config.
If the new master can be caught up from the WAL then the master
gets promoted to VOTER else it remains as NON_VOTER without master tablet
copying support.
Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
---
M src/kudu/common/wire_protocol.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/tool_action_master.cc
20 files changed, 800 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/16321/3
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
[master] KUDU-2181 Raft ChangeConfig request to add a master
This change:
- Adds hidden feature flag "--master_support_change_config" off by
default.
- RPC changes to add a master that initiates Raft config change and
responds asynchronously.
- RPC changes to report back member type(VOTER/NON_VOTER) of masters
- Removes the cached master_addresses in catalog manager as it's no
longer static.
- Updates and adds comments in MasterOptions such that it's used to
fetch master addresses only during master init time as masters can
be added/removed dynamically with this change.
- Updates ListMasters() to look at local Raft config instead of
MasterOptions as the masters can be added/removed dynamically.
If the new master can be caught up from the WAL then the master
gets promoted to VOTER else it remains as NON_VOTER without master
tablet copying support.
Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Reviewed-on: http://gerrit.cloudera.org:8080/16321
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Bankim Bhavsar <ba...@cloudera.com>
---
M src/kudu/common/wire_protocol.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/tool_action_master.cc
20 files changed, 987 insertions(+), 56 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Bankim Bhavsar: Verified
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 9
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 8
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 24 Sep 2020 01:29:46 +0000
Gerrit-HasComments: No
[kudu-CR] [master] KUDU-2181 Raft change config request for adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft change config request for adding a master
......................................................................
Patch Set 3:
> Patch Set 3:
>
> Build Started http://jenkins.kudu.apache.org/job/kudu-gerrit/22385/
One issue I realized is that --master_addressses for non-leader master doesn't get updated in my changes so far. I need to think about how to solve that.
Maintaining separate in-memory state and the Raft config state for masters is definitely tricky and I do recall pointing out in the design that getting rid of the in-memory state might be an option instead of trying to keep them in-sync.
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 12 Sep 2020 00:40:14 +0000
Gerrit-HasComments: No
[kudu-CR] [master] KUDU-2181 Raft change config request for adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft change config request for adding a master
......................................................................
Patch Set 4:
(17 comments)
Thanks for the review, Alexey!
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/common/wire_protocol.proto
File src/kudu/common/wire_protocol.proto:
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/common/wire_protocol.proto@127
PS4, Line 127: be
> drop
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@784
PS4, Line 784: std::pair<consensus::RaftPeerPB::Role, consensus::RaftPeerPB::MemberType>
> nit: does it make sense to add a typedef for this?
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@812
PS4, Line 812: bool add
> Consider introducing an enum for this: it improves the readability of the c
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@1119
PS4, Line 1119: consensus
> Is it possible to reuse CatalogManager::master_consensus() instead of intro
master_consensus() is used by catalog manager when it may not be fully initialized.
So can't use that function.
I agree having an additional consensus function could cause confusion. My goal was to put common code between Role() and GetRoleAndMemberType() in a common function but it's just a couple of lines. So removing it instead as simply checking for existing IsInitialized() function looks sufficient.
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@1119
PS4, Line 1119: const
> Drop 'const' because this is not a truly constant method (you can reset the
See below.
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@154
PS4, Line 154: "--master_addresses=" + JoinStrings(master_addresses, ","),
: "--rpc_reuseport=true",
: "--master_support_change_config",
: "--master_address_add_new_master=" + reserved_hp_str_,
: "--logtostderr",
: "--logbuflevel=-1"
> Does it make sense to retrieve the 'common' part of options for master from
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@202
PS4, Line 202: CHECK_OK
> Would RETURN_NOT_OK() suffice here?
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@240
PS4, Line 240: StartCluster
> nit: wrap into NO_FATALS() or make StartCluster() return status and wrap in
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@243
PS4, Line 243: master
> drop
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@253
PS4, Line 253: AddMasterToCluster
> Does it make sense to verify that an attempt of initiating master config ch
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@261
PS4, Line 261: rpc::RpcController
> nit here and elsewhere: it's possible to add 'using kudu::rpc::RpcControlle
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@262
PS4, Line 262: leader_master
> nit: maybe, leader_master_idx would be more appropriate name for this varia
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@270
PS4, Line 270: ASSERT_TRUE(master.role() == consensus::RaftPeerPB::LEADER ||
: master.role() == consensus::RaftPeerPB::FOLLOWER);
: }
> Does it make sense to add a sanity check to make sure there is only one lea
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@287
PS4, Line 287: Adding the same master
> Does is make sense to verify and an attempt to add any of the former master
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@295
PS4, Line 295: ASSERT_OK(cluster_->GetLeaderMasterIndex(&leader_master));
: ASSERT_TRUE(
: cluster_->master_proxy(leader_master)->AddMaster(req, &resp, &rpc).IsRemoteError());
> Here and elsewhere, the leader master might change in between: this might b
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@432
PS4, Line 432: TestAddMasterWithNoLastKnownAddr
> In addition, does it make sense to add a test to verify that that the syste
Done
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/master.proto@913
PS4, Line 913: required
> I think this should be 'optional' as in other requests because we are plann
Done
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 17 Sep 2020 17:25:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
Patch Set 5:
(15 comments)
Just nits for the most part.
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.h@810
PS5, Line 810: kRemoveMaster
This will be tested in a follow-up patch, right?
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.cc@1419
PS5, Line 1419:
nit: 4 spaces here
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@126
PS5, Line 126: resp
Should we also check resp.error() here? Same elsewhere.
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@149
PS5, Line 149: // Start with an existing master daemon.
nit: maybe, "Start with an existing master daemon's options, but modify them for use in a new master"?
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@186
PS5, Line 186: ASSERT_EVENTUALLY([&] {
nit: why do we need this?
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@194
PS5, Line 194: num_masters_
nit: this number changes throughout the test, so it isn't obvious what its value should be. E.g. should it include the new master?
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@198
PS5, Line 198: returning the status.
nit: could you mention how this might fail, so it's obvious when and why we need to ASSERT_EVENTUALLY?
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@258
PS5, Line 258: ASSERT_EVENTUALLY
nit: in general, it isn't obvious why we need ASSERT_EVENTUALLYs everywhere. Could you get by using fewer of them, or comment why they are necessary at each usage? If we put too much into these blocks, they may hide some bugs.
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@273
PS5, Line 273: num_masters_
nit: I'd much rather have this be constant and explicitly use `orig_num_masters + 1` where appropriate. That way it's obvious to readers no matter where in the test they are what the value is.
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@399
PS5, Line 399: SleepFor(MonoDelta::FromSeconds(1));
nit: could we instead look at metrics to wait until WAL GC has happened?
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@515
PS5, Line 515: });
nit: could we also run VerifyNumMastersAndGetAddresses here too? Same with the other negative tests?
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.h
File src/kudu/master/master.h:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.h@92
PS5, Line 92: MasterOptions* mutable_opts() { return &opts_; }
Is this needed still?
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc@64
PS5, Line 64: namespace kudu {
: namespace rpc {
: class RpcContext;
: } // namespace rpc
: } // namespace kudu
nit: can you move this down into the other kudu namespace?
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc@424
PS5, Line 424: auto
nit: could be a const ref
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master_service.cc@267
PS5, Line 267: InitiateMasterChange
nit: InitiateMasterChangeConfig
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 21 Sep 2020 19:47:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
Patch Set 6:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/catalog_manager.cc@1615
PS6, Line 1615: optional<const string&> user = rpc ?
> error: no viable conversion from 'optional<typename boost::decay<const basi
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/catalog_manager.cc@1848
PS6, Line 1848: req.has_dimension_label() ? make_optional<string>(req.dimension_label()) : none;
> error: no matching function for call to 'make_optional' [clang-diagnostic-e
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/catalog_manager.cc@2146
PS6, Line 2146: optional<const string&> user = rpc ?
> error: no viable conversion from 'optional<typename boost::decay<const basi
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/catalog_manager.cc@2506
PS6, Line 2506: make_optional<string>(step.add_range_partition().dimension_label()) : none;
> error: no matching function for call to 'make_optional' [clang-diagnostic-e
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/catalog_manager.cc@2581
PS6, Line 2581: optional<const string&> user = rpc ?
> error: no viable conversion from 'optional<typename boost::decay<const basi
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/catalog_manager.cc@4822
PS6, Line 4822: make_optional<string>(old_info.pb.dimension_label()) : none;
> error: no matching function for call to 'make_optional' [clang-diagnostic-e
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/dynamic_multi_master-test.cc@162
PS6, Line 162: }
> Nit: Would it make sense to define a deadline instead and keep retrying unt
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/dynamic_multi_master-test.cc@177
PS6, Line 177: return std::make_pair(std::move(s), std::move(err_code));
> warning: std::move of the variable 'err_code' of the trivially-copyable typ
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/dynamic_multi_master-test.cc@263
PS6, Line 263: return std::make_pair(std::move(s), std::move(err_code));
> warning: std::move of the variable 'err_code' of the trivially-copyable typ
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/dynamic_multi_master-test.cc@266
PS6, Line 266: return RunLeaderMasterRPC(add_master).AndThen([&] {
> Nit: AndThen is useful when we need to do something extra with the result o
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/dynamic_multi_master-test.cc@399
PS6, Line 399: }
> The log_gc_duration histogram metric has a TotalCount() method that might b
Done. Thanks!
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc@424
PS5, Line 424: auto
> Missed this?
Indeed. Done.
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/master_service.cc@435
PS6, Line 435: make_optional<const string&>(rpc->remote_user().username()));
> error: no viable conversion from 'optional<typename boost::decay<const basi
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/master_service.cc@474
PS6, Line 474: req, resp, make_optional<const string&>(rpc->remote_user().username()));
> error: no viable conversion from 'optional<typename boost::decay<const basi
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/master_service.cc@514
PS6, Line 514: req, resp, make_optional<const string&>(rpc->remote_user().username()));
> error: no viable conversion from 'optional<typename boost::decay<const basi
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/master_service.cc@528
PS6, Line 528: req, resp, make_optional<const string&>(rpc->remote_user().username()));
> error: no viable conversion from 'optional<typename boost::decay<const basi
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/master_service.cc@541
PS6, Line 541: req, resp, make_optional<const string&>(rpc->remote_user().username()));
> error: no viable conversion from 'optional<typename boost::decay<const basi
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/master_service.cc@563
PS6, Line 563: req, resp, make_optional<const string&>(rpc->remote_user().username()));
> error: no viable conversion from 'optional<typename boost::decay<const basi
Done
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/master_service.cc@581
PS6, Line 581: req, resp, make_optional<const string&>(rpc->remote_user().username()),
> error: no viable conversion from 'optional<typename boost::decay<const basi
Done
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 6
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Sep 2020 22:38:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft change config request for adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft change config request for adding a master
......................................................................
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/consensus/raft_consensus.cc@2470
PS2, Line 2470: for (const auto& peer : cmeta_->ActiveConfig().peers()) {
> nit: maybe call this 'raft_config' or something to reflect what its type is
Done
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.h@781
PS2, Line 781: talogManager's role and member type in a consen
> nit: are there DCHECK()s we can add to enforce this?
I simply copied the implementation and comment from the function "Role" above.
Implementation checks whether catalog is running and returns UNKNOWN_ROLE/MEMBER_TYPE if not initialized. So updated the comments instead.
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc@5564
PS2, Line 5564: auto comp_promise_ptr = s
> If this is not OK, how should we expect users to catch such errors? Is ther
ChangeConfig() implementation in Raft is inherently async, so earlier thought was that clients can check with ListMasters call. See the comment in master.proto. But I see your point about informing clients about the final outcome.
Adding/removing masters will be rare and I think it's better to make the call synchronous with some timeout instead as I don't expect clients/tool to do other async work while master addition/removal is in progress and clients would know the final status.
So updated the code accordingly with 10 sec timeout which looks reasonable for such an operation though I haven't dived into the RPC implementation to determine whether that's okay.
I looked around a bit but couldn't find existing examples of RPCs wherein the server returns the response to the client in an async fashion.
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/dynamic_multi_master-test.cc@148
PS2, Line 148: const string new_master_id = Substitute("master-$0", num_masters_);
: new_master_opts.wal_dir = cluster_->GetWalPath(new_master_id);
: new_master_opts.data_dirs = cluster_->GetDataPaths(new_master_id);
: new_master_opts.log_dir = cluster_->GetLogPath(new_master_id);
: new_master_opts.rpc_bind_address = reserved_hp_;
: new_master_opts.extra_flags = {
: "--master_addresses=" + JoinStrings(master_addresses, ","),
: "--rpc_reuseport=true",
> Could we just copy some of the opts from an existing daemon? Seems then we'
Done
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master.h
File src/kudu/master/master.h:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master.h@89
PS2, Line 89: MasterOptions*
> nit: consider returning a pointer instead for mutable accessors. That's the
Done
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_options.h
File src/kudu/master/master_options.h:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_options.h@57
PS2, Line 57:
: // Allows setting/overwriting list of masters.
> A static lock is incredibly surprising for things that aren't designed to b
Done
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc@243
PS2, Line 243: if (!FLAGS_master_support_change_config) {
: rpc->RespondFailure(Status::NotSupported("Adding master is not supported"));
: return;
: }
> Just FYI this can also be done via MasterServiceImpl::SupportsFeature()
Thanks for the pointer. Added the feature in MasterFeatures as it helps with forward compatibility (newer clients communicating with older master server).
I think this check is still needed. What if the client doesn't set the required feature and the feature is not yet complete and hence disabled in the server?
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc@255
PS2, Line 255: ERRO
> nit: users should probably be concerned about this, so this should probably
Done
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 12 Sep 2020 00:34:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@258
PS5, Line 258: *req.mutabl
> Done
Thanks! Much clearer this time around
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/dynamic_multi_master-test.cc@162
PS6, Line 162: }
Nit: Would it make sense to define a deadline instead and keep retrying until then?
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/dynamic_multi_master-test.cc@266
PS6, Line 266: return RunLeaderMasterRPC(add_master).AndThen([&] {
Nit: AndThen is useful when we need to do something extra with the result of the first error we see (eg check for specific errors). If we're just returning the error, it's simpler to just use RETURN_NOT_OK() and return.
http://gerrit.cloudera.org:8080/#/c/16321/6/src/kudu/master/dynamic_multi_master-test.cc@399
PS6, Line 399: }
The log_gc_duration histogram metric has a TotalCount() method that might be useful.
I use it here: https://gerrit.cloudera.org/c/16492/3/src/kudu/integration-tests/txn_participant-itest.cc
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc@424
PS5, Line 424: auto
> nit: could be a const ref
Missed this?
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 6
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Sep 2020 19:46:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master]: Initiate change config request for adding master
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: WIP [master]: Initiate change config request for adding master
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/consensus/raft_consensus.cc@2467
PS1, Line 2467: const auto& consensus = cmeta_->ActiveConfig();
: auto member_type = RaftPeerPB::UNKNOWN_MEMBER_TYPE;
: const auto& local_peer_uuid = peer_uuid();
Can we do any of these outside the lock?
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/catalog_manager.cc@5491
PS1, Line 5491: if (!submission_status.ok()) {
: // Caller will log the submission_status error, this is for the additional error_code.
: VLOG(1) << Substitute("Failed initiating ChangeConfig request to $0 master, error: $1",
: add_remove_str,
: error_code ? TabletServerErrorPB::Code_Name(*error_code) : "unknown");
: }
Rather than logging, how about passing back the error code via RETURN_NOT_OK_PREPEND?
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/dynamic_multi_master-test.cc@79
PS1, Line 79: opts.bind_mode = BindMode::LOOPBACK;
Why not just leave the default?
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/dynamic_multi_master-test.cc@173
PS1, Line 173: // since the system catalog is essentially empty.
Perhaps consider creating a table at least so we can verify the table gets replicated upon catching the new master up. With so little in the WAL, we shouldn't have to worry about GC here.
It'd also be good to verify that after adding the master, if we create a table, the new master's catalog also gets updated, or better yet, force the new master to become the leader.
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/dynamic_multi_master-test.cc@186
PS1, Line 186: }
: });
To be sure the config change is persisted, how about restarting the cluster after this and trying to list the masters again?
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 11 Aug 2020 17:59:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
Patch Set 7: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16321/7/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/7/src/kudu/master/dynamic_multi_master-test.cc@141
PS7, Line 141: IllegalStatus
nit: IllegalState
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 7
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Sep 2020 23:09:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16321
to look at the new patch set (#5).
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
[master] KUDU-2181 Raft ChangeConfig request to add a master
This change:
- Adds hidden feature flag "--master_support_change_config" off by
default.
- RPC changes to add a master that initiates Raft config change and
responds asynchronously.
- RPC changes to report back member type(VOTER/NON_VOTER) of masters
- Removes the cached master_addresses in catalog manager as it's no
longer static.
- Updates and adds comments in MasterOptions such that it's used to
fetch master addresses only during master init time as masters can
be added/removed dynamically with this change.
- Updates ListMasters() to look at local Raft config instead of
MasterOptions as the masters can be added/removed dynamically.
If the new master can be caught up from the WAL then the master
gets promoted to VOTER else it remains as NON_VOTER without master
tablet copying support.
Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
---
M src/kudu/common/wire_protocol.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/tool_action_master.cc
20 files changed, 869 insertions(+), 55 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/16321/5
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [master] KUDU-2181 Raft change config request for adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16321
to look at the new patch set (#2).
Change subject: [master] KUDU-2181 Raft change config request for adding a master
......................................................................
[master] KUDU-2181 Raft change config request for adding a master
Change includes:
- Hidden feature flag "--master_support_change_config" off by default
- RPC changes to add a master that initiates Raft config change
- RPC changes to report back member type(VOTER/NON_VOTER) of masters
- Initiates ChangeConfig request to add a master to the master Raft config
- Removes the cached master_addresses in catalog manager as it's no longer
static.
- Updates the master_addresses in MasterOptions on successful update to
master Raft config.
If the new master can be caught up from the WAL then the master
gets promoted to VOTER else it remains as NON_VOTER without master tablet
copying support.
Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
---
M src/kudu/common/wire_protocol.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/tool_action_master.cc
20 files changed, 765 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/16321/2
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16321
to look at the new patch set (#8).
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
[master] KUDU-2181 Raft ChangeConfig request to add a master
This change:
- Adds hidden feature flag "--master_support_change_config" off by
default.
- RPC changes to add a master that initiates Raft config change and
responds asynchronously.
- RPC changes to report back member type(VOTER/NON_VOTER) of masters
- Removes the cached master_addresses in catalog manager as it's no
longer static.
- Updates and adds comments in MasterOptions such that it's used to
fetch master addresses only during master init time as masters can
be added/removed dynamically with this change.
- Updates ListMasters() to look at local Raft config instead of
MasterOptions as the masters can be added/removed dynamically.
If the new master can be caught up from the WAL then the master
gets promoted to VOTER else it remains as NON_VOTER without master
tablet copying support.
Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
---
M src/kudu/common/wire_protocol.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/tool_action_master.cc
20 files changed, 987 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/16321/8
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 8
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16321/7/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/7/src/kudu/master/dynamic_multi_master-test.cc@141
PS7, Line 141: IllegalState
> nit: IllegalState
Done
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 8
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 24 Sep 2020 01:01:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16321
to look at the new patch set (#7).
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
[master] KUDU-2181 Raft ChangeConfig request to add a master
This change:
- Adds hidden feature flag "--master_support_change_config" off by
default.
- RPC changes to add a master that initiates Raft config change and
responds asynchronously.
- RPC changes to report back member type(VOTER/NON_VOTER) of masters
- Removes the cached master_addresses in catalog manager as it's no
longer static.
- Updates and adds comments in MasterOptions such that it's used to
fetch master addresses only during master init time as masters can
be added/removed dynamically with this change.
- Updates ListMasters() to look at local Raft config instead of
MasterOptions as the masters can be added/removed dynamically.
If the new master can be caught up from the WAL then the master
gets promoted to VOTER else it remains as NON_VOTER without master
tablet copying support.
Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
---
M src/kudu/common/wire_protocol.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/tool_action_master.cc
20 files changed, 987 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/16321/7
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 7
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [master] KUDU-2181 Raft change config request for adding a master
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft change config request for adding a master
......................................................................
Patch Set 3:
(4 comments)
Just a quick pass over + responding to comments
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc@5564
PS2, Line 5564: auto comp_promise_ptr = s
> ChangeConfig() implementation in Raft is inherently async, so earlier thoug
See usages of RpcOpCompletionCallback in https://github.com/apache/kudu/blob/master/src/kudu/tserver/tablet_service.cc for how we do this on tservers for AlterSchema and Write requests.
I don't mind making it synchronous. Might be worth passing in some RPC timeout from the RpcContext in MasterServiceImpl.
http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.h
File src/kudu/master/master_options.h:
http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.h@51
PS3, Line 51: // Allows setting/overwriting list of masters. Only to be used by tests.
: void SetMasterAddressesForTests(std::vector<HostPort> addresses) {
: SetMasterAddresses(std::move(addresses));
: }
What's the point of specifying this if it's the same as SetMasterAddresses()? Why not just make SetMasterAddresses() public?
http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.cc
File src/kudu/master/master_options.cc:
http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.cc@110
PS3, Line 110: if (std::find(master_addresses_.begin(), master_addresses_.end(), hp) !=
: master_addresses_.end()) {
: return Status::AlreadyPresent(Substitute("Master $0 already present", hp.ToString()));
: }
:
: master_addresses_.emplace_back(hp);
nit: this could be simplified with EmplaceIfNotPresent() from gutil/map-util.h
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc@243
PS2, Line 243: if (!FLAGS_master_support_change_config) {
: rpc->RespondFailure(Status::NotSupported("Adding master is not supported"));
: return;
: }
> Thanks for the pointer. Added the feature in MasterFeatures as it helps wit
Yeah, we should probably only add SupportsFeature() once the feature is complete. Until then, I'd be ok leaving this here.
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 12 Sep 2020 01:26:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
Patch Set 8: Verified+1
Unrelated ASAN test failure in SlowTabletBootstrapTest.TestFallBehindSlowBootstrap
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 8
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 24 Sep 2020 05:31:54 +0000
Gerrit-HasComments: No
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
Patch Set 5:
(4 comments)
Some more nits.
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.h@810
PS5, Line 810: kRemoveMaster
> This will be tested in a follow-up patch, right?
+1
It would be great to explicitly mention this in the commit description (if the idea is to add corresponding tests in a follow-up patch indeed).
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@186
PS5, Line 186: ASSERT_EVENTUALLY([&] {
> nit: why do we need this?
I guess the idea was to protect against fluctuations of master leadership (at least I mentioned that in one of the comments in PS4, and I mentioned ASSERT_EVENTUALLY might help there).
Maybe, instead of using ASSERT_EVENTUALLY on any failure, these should be handling MasterErrorPB::NOT_THE_LEADER failures only?
Another approach could be using some sort of a function that finds the leader master and calls the specified functor, retrying the operation if MasterErrorPB::NOT_THE_LEADER is returned back?
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@518
PS5, Line 518: a master with missing master address
Not sure whether I missed it or not, but what about the case of trying to add a master which is unreachable (e.g., currently shutdown or outright non-routable IP address)?
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@524
PS5, Line 524: master
nit: drop
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 21 Sep 2020 22:58:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft change config request for adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft change config request for adding a master
......................................................................
Patch Set 3:
(3 comments)
> Patch Set 3:
>
> > Patch Set 3:
> >
> > Build Started http://jenkins.kudu.apache.org/job/kudu-gerrit/22385/
>
> One issue I realized is that --master_addressses for non-leader master doesn't get updated in my changes so far. I need to think about how to solve that.
>
> Maintaining separate in-memory state and the Raft config state for masters is definitely tricky and I do recall pointing out in the design that getting rid of the in-memory state might be an option instead of trying to keep them in-sync.
Updated the code such that master_addresses_ in MasterOptions is not updated on addition/removal of masters dynamically. MasterOptions is okay to use during master/catalog_manager init time but not after that. Updated comments in MasterOptions accordingly and mentioned using existing Master::GetMasterHostPorts() that fetches master addresses from local Raft config instead.
This helps solve the problem with PS3 wherein only the current leader master had updated master addresses in MasterOptions.
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc@5564
PS2, Line 5564: auto comp_promise_ptr = s
> See usages of RpcOpCompletionCallback in https://github.com/apache/kudu/blo
Thanks for the pointer.
It's simple enough to pass the RpcContext to the completion callback to Raft which allows releasing the worker thread serving the RPC request sooner. So from the client point of view it's still synchronous but avoids blocking the RPC worker thread while the ChangeConfig request is being processed by the consensus engine.
http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.h
File src/kudu/master/master_options.h:
http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.h@51
PS3, Line 51: // Allows setting/overwriting list of masters. Only to be used by tests.
: void SetMasterAddressesForTests(std::vector<HostPort> addresses) {
: SetMasterAddresses(std::move(addresses));
: }
> What's the point of specifying this if it's the same as SetMasterAddresses(
Prevent callers from invoking SetMasterAddresses() if the function name clearly mentions "Tests".
This is not needed anymore.
http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.cc
File src/kudu/master/master_options.cc:
http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.cc@110
PS3, Line 110: if (std::find(master_addresses_.begin(), master_addresses_.end(), hp) !=
: master_addresses_.end()) {
: return Status::AlreadyPresent(Substitute("Master $0 already present", hp.ToString()));
: }
:
: master_addresses_.emplace_back(hp);
> nit: this could be simplified with EmplaceIfNotPresent() from gutil/map-uti
master_addresses_ is a vector, so couldn't use this.
AddMaster() function is removed, so this is not needed.
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 15 Sep 2020 21:55:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft change config request for adding a master
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft change config request for adding a master
......................................................................
Patch Set 4:
(18 comments)
I looked through quickly. Overall looks good, I think I'll do a second pass tomorrow.
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/common/wire_protocol.proto
File src/kudu/common/wire_protocol.proto:
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/common/wire_protocol.proto@127
PS4, Line 127: be
drop
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@784
PS4, Line 784: std::pair<consensus::RaftPeerPB::Role, consensus::RaftPeerPB::MemberType>
nit: does it make sense to add a typedef for this?
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@812
PS4, Line 812: bool add
Consider introducing an enum for this: it improves the readability of the code at call sites and brings more consistency to the signature of the method.
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@1119
PS4, Line 1119: const
Drop 'const' because this is not a truly constant method (you can reset the underlying pointer given the returned shared_ptr). See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/const.md for details.
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@1119
PS4, Line 1119: consensus
Is it possible to reuse CatalogManager::master_consensus() instead of introducing this new method?
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@154
PS4, Line 154: "--master_addresses=" + JoinStrings(master_addresses, ","),
: "--rpc_reuseport=true",
: "--master_support_change_config",
: "--master_address_add_new_master=" + reserved_hp_str_,
: "--logtostderr",
: "--logbuflevel=-1"
Does it make sense to retrieve the 'common' part of options for master from opts_.extra_master_flags ?
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@202
PS4, Line 202: CHECK_OK
Would RETURN_NOT_OK() suffice here?
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@240
PS4, Line 240: StartCluster
nit: wrap into NO_FATALS() or make StartCluster() return status and wrap into ASSERT_OK() to bail from the test scenario earlier if an error occurs within StartCluster()?
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@243
PS4, Line 243: master
drop
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@245
PS4, Line 245: VerifyNumMastersAndGetAddresses
nit: wrap into NO_FATALS()
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@253
PS4, Line 253: AddMasterToCluster
Does it make sense to verify that an attempt of initiating master config change via a non-leader master returns an error, as expected?
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@261
PS4, Line 261: rpc::RpcController
nit here and elsewhere: it's possible to add 'using kudu::rpc::RpcController;' and the omit 'rpc' namespace prefix throughout the code in this file
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@262
PS4, Line 262: leader_master
nit: maybe, leader_master_idx would be more appropriate name for this variable?
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@270
PS4, Line 270: ASSERT_TRUE(master.role() == consensus::RaftPeerPB::LEADER ||
: master.role() == consensus::RaftPeerPB::FOLLOWER);
: }
Does it make sense to add a sanity check to make sure there is only one leader in the list?
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@287
PS4, Line 287: Adding the same master
Does is make sense to verify and an attempt to add any of the former masters returns an error as well?
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@295
PS4, Line 295: ASSERT_OK(cluster_->GetLeaderMasterIndex(&leader_master));
: ASSERT_TRUE(
: cluster_->master_proxy(leader_master)->AddMaster(req, &resp, &rpc).IsRemoteError());
Here and elsewhere, the leader master might change in between: this might be a source of flakiness in the future (especially in TSAN builds). Probably, it makes sense to wrap this into some sort of ASSERT_EVENTUALLY() or alike.
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@432
PS4, Line 432: TestAddMasterWithNoLastKnownAddr
In addition, does it make sense to add a test to verify that that the system behaves as expected when '--master_support_change_config' is omitted or set to 'false'?
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/master.proto@913
PS4, Line 913: required
I think this should be 'optional' as in other requests because we are planning to upgrade to proto3 eventually.
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 16 Sep 2020 02:52:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft change config request for adding a master
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft change config request for adding a master
......................................................................
Patch Set 2:
(9 comments)
Did a high-level first pass so far. Didn't dig too deeply into the tests, but overall the approach seems sound.
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/consensus/raft_consensus.cc@2470
PS2, Line 2470: const auto& consensus = cmeta_->ActiveConfig();
nit: maybe call this 'raft_config' or something to reflect what its type is.
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/consensus/raft_consensus.cc@2474
PS2, Line 2474: break;
: }
: }
Would it make sense to make this function return a Status and return NotFound() if the local UUID is not in the Raft config?
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.h@781
PS2, Line 781: must be initialized before calling this method.
nit: are there DCHECK()s we can add to enforce this?
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc@5564
PS2, Line 5564: if (comp_status.ok()) {
If this is not OK, how should we expect users to catch such errors? Is there any way we can make error-handling more straightforward, e.g. passing a callback from the MasterServiceImpl to call upon success/failure to return to the client?
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/dynamic_multi_master-test.cc@148
PS2, Line 148: new_master_opts.exe = cluster_->GetBinaryPath("kudu-master");
: new_master_opts.messenger = cluster_->messenger();
: new_master_opts.block_manager_type = cluster_->block_manager_type();
: new_master_opts.wal_dir = cluster_->GetWalPath(new_master_id);
: new_master_opts.data_dirs = cluster_->GetDataPaths(new_master_id);
: new_master_opts.log_dir = cluster_->GetLogPath(new_master_id);
: new_master_opts.rpc_bind_address = reserved_hp_;
: new_master_opts.start_process_timeout = cluster_->start_process_timeout();
Could we just copy some of the opts from an existing daemon? Seems then we'd only need to set wal_dir, data_dir, log_dir, rpc_bind_address, and extra_flags
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master.h
File src/kudu/master/master.h:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master.h@89
PS2, Line 89: MasterOptions&
nit: consider returning a pointer instead for mutable accessors. That's the status quo for auto-generated protobuf at least.
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_options.h
File src/kudu/master/master_options.h:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_options.h@57
PS2, Line 57: // Locks are not copyable, so using a static lock instead. MasterOptions is used during init
: // time, so apart from tests there aren't usually multiple instances of MasterOptions.
A static lock is incredibly surprising for things that aren't designed to be singletons. It seems wrong to share static mutable state across multiple instances of this class. If we need to copy a class, why not define a copy constructor?
Also a simple_spinlock seems like it would be sufficient.
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc@243
PS2, Line 243: if (!FLAGS_master_support_change_config) {
: rpc->RespondFailure(Status::NotSupported("Adding master is not supported"));
: return;
: }
Just FYI this can also be done via MasterServiceImpl::SupportsFeature()
http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc@255
PS2, Line 255: INFO
nit: users should probably be concerned about this, so this should probably be a warning or error.
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 10 Sep 2020 21:31:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master]: Initiate change config request for adding master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: WIP [master]: Initiate change config request for adding master
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/consensus/raft_consensus.cc@2467
PS1, Line 2467: const auto& consensus = cmeta_->ActiveConfig();
: auto member_type = RaftPeerPB::UNKNOWN_MEMBER_TYPE;
: const auto& local_peer_uuid = peer_uuid();
> Can we do any of these outside the lock?
ActiveConfig() returns a const reference. So getting access to the reference using const ptr under lock and then accessing the consensus state wouldn't be thread safe. The thread safe option is creating of copy under the lock and then using the local copy to access members of consensus without lock.
However RaftConsensusPB looks like a fairly large object so copy won't be cheap. So considering lock is held for iterating over small number of peers(typically 3) and the copy won't be cheap, holding lock looks reasonable.
Moved couple of variable declarations above the lock.
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/catalog_manager.cc@5491
PS1, Line 5491: if (!submission_status.ok()) {
: // Caller will log the submission_status error, this is for the additional error_code.
: VLOG(1) << Substitute("Failed initiating ChangeConfig request to $0 master, error: $1",
: add_remove_str,
: error_code ? TabletServerErrorPB::Code_Name(*error_code) : "unknown");
: }
> Rather than logging, how about passing back the error code via RETURN_NOT_O
Done
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/dynamic_multi_master-test.cc@79
PS1, Line 79: opts.bind_mode = BindMode::LOOPBACK;
> Why not just leave the default?
Done
http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/dynamic_multi_master-test.cc@186
PS1, Line 186: }
: });
> To be sure the config change is persisted, how about restarting the cluster
Done
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 02 Sep 2020 17:24:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft change config request for adding a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16321
to look at the new patch set (#4).
Change subject: [master] KUDU-2181 Raft change config request for adding a master
......................................................................
[master] KUDU-2181 Raft change config request for adding a master
This change:
- Adds hidden feature flag "--master_support_change_config" off by default
- RPC changes to add a master that initiates Raft config change and
responds asynchronously.
- RPC changes to report back member type(VOTER/NON_VOTER) of masters
- Removes the cached master_addresses in catalog manager as it's no longer
static.
- Updates and adds comments in MasterOptions such that it's used to fetch
master addresses only during master init time as masters can be added/removed
dynamically with this change.
- Updates ListMasters() to look at local Raft config instead of MasterOptions
as the masters can be added/removed dynamically.
If the new master can be caught up from the WAL then the master
gets promoted to VOTER else it remains as NON_VOTER without master tablet
copying support.
Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
---
M src/kudu/common/wire_protocol.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/dynamic_multi_master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_options.cc
M src/kudu/master/master_options.h
M src/kudu/master/master_path_handlers.cc
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/tool_action_master.cc
20 files changed, 756 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/16321/4
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
Patch Set 5:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.h@810
PS5, Line 810: kRemoveMaster
> +1
Remove master is not implemented. Removing the enum.
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.cc@1419
PS5, Line 1419:
> nit: 4 spaces here
Done
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@126
PS5, Line 126: resp
> Should we also check resp.error() here? Same elsewhere.
Done
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@149
PS5, Line 149: // Start with an existing master daemon.
> nit: maybe, "Start with an existing master daemon's options, but modify the
Done
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@186
PS5, Line 186: ASSERT_EVENTUALLY([&] {
> I guess the idea was to protect against fluctuations of master leadership (
Yeah it was to prevent flakiness in tests due to leadership change between querying for leader master and issuing leader master RPC.
Added a functor approach instead.
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@194
PS5, Line 194: num_masters_
> nit: this number changes throughout the test, so it isn't obvious what its
Changed to orig_num_masters_ and check using orig_num_masters_ as the base.
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@198
PS5, Line 198: returning the status.
> nit: could you mention how this might fail, so it's obvious when and why we
Done
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@258
PS5, Line 258: ASSERT_EVENTUALLY
> nit: in general, it isn't obvious why we need ASSERT_EVENTUALLYs everywhere
Done
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@273
PS5, Line 273: num_masters_
> nit: I'd much rather have this be constant and explicitly use `orig_num_mas
Done
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@399
PS5, Line 399: SleepFor(MonoDelta::FromSeconds(1));
> nit: could we instead look at metrics to wait until WAL GC has happened?
I took a look around but couldn't determine the right metric that would suggest sys catalog WAL has been GC'ed or what it's current size is to determine that event has happened.
Added TODO and keeping the sleep here for now.
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@515
PS5, Line 515: });
> nit: could we also run VerifyNumMastersAndGetAddresses here too? Same with
Done
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@518
PS5, Line 518: a master with missing master address
> Not sure whether I missed it or not, but what about the case of trying to a
Done
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@524
PS5, Line 524: master
> nit: drop
Done
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.h
File src/kudu/master/master.h:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.h@92
PS5, Line 92: MasterOptions* mutable_opts() { return &opts_; }
> Is this needed still?
Good catch.
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc@64
PS5, Line 64: namespace kudu {
: namespace rpc {
: class RpcContext;
: } // namespace rpc
: } // namespace kudu
> nit: can you move this down into the other kudu namespace?
Done
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master_service.cc@267
PS5, Line 267: InitiateMasterChange
> nit: InitiateMasterChangeConfig
Done
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Sep 2020 16:31:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has removed a vote on this change.
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c
Gerrit-Change-Number: 16321
Gerrit-PatchSet: 8
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)