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/14 19:15:01 UTC
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Bankim Bhavsar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16340
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
[master] KUDU-3182 Allow single master to specify --master_addresses
'last_known_addr' field is not persisted for a single master Raft
configuration. This is okay as long as we have a single master
configuration. On dynamically transitioning from single master
to two master configuration, ChangeConfig() request to ADD_PEER
fails the validation in VerifyRaftConfig().
Inorder to transition to multi-master configuration, 'last_known_addr'
must be specified.
Some options considered were using the --rpc_bind_addresses flag, if
specified. If not, use GetHostName(). None of these are reliable.
So it's best to let user to specify the address of the single master
as is the case for specifying master address in tablet servers.
Currently --master_addresses flag must be empty for single master
configuration and --master_addresses with single address is not
permitted.
So the idea is to use existing --master_addresses flag for single
master configuration too and not just distributed configuration.
This change allows specifying --master_addresses flag for
single master configuration. To be compatible with existing
deployments, specifying this flag for single master configurations
is still optional and not required. If specified, it's used
to set the 'last_known_addr' field in the master Raft configuration.
This change is a pre-requisite for upcoming dynamic multi-master
changes.
Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
A src/kudu/master/master_options-test.cc
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/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.cc
9 files changed, 155 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/16340/1
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc@57
PS1, Line 57: using strings::Substitute;
:
> Re: forward compatibility ie. older code reading Raft config persisted by n
Done
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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-Comment-Date: Wed, 26 Aug 2020 23:22:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc@57
PS1, Line 57: class MasterOptionsTest : public KuduTest {
: };
> Will existing masters be able to specify this flag and update their Raft co
No, the current change doesn't update Raft config for existing deployments by simply specifying this flag.
I was thinking of keeping that separate using 2 possible options:
- Add a RPC to allow updating config without any downtime
- Use existing rewrite_raft_config tool to populate the last_known_addr field. Though this'll incur downtime.
Though looking at the code more, like you mention, it should be possible to add 'last_known_addr' field in Raft config on kudu startup for existing deployments provided user has supplied the single master address.
If customers use a config mgmt tool like Cloudera Manager then with updates with kudu startup script --master_addresses field will be populated and the kudu master restart after the upgrade will populate the 'last_known_addr' field. So technically this can't be considered additional downtime to add a field.
One minor downside, I see, is that for every single master restart with --master_addresses field specified, Raft config will need to be checked for existence of the field.
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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: Fri, 14 Aug 2020 22:09:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16340/4/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
http://gerrit.cloudera.org:8080/#/c/16340/4/src/kudu/master/sys_catalog.h@360
PS4, Line 360: // For a single master config, verifies the on-disk Raft config and populates the
: // 'last_known_addr' in the on-disk Raft config, if master address is sp
> nit: Could you make it more obvious that this may update on-disk state?
Done
http://gerrit.cloudera.org:8080/#/c/16340/4/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/16340/4/src/kudu/master/sys_catalog.cc@198
PS4, Line 198: ftConfigPB config = cmeta->CommittedCon
> nit: it's a little misleading to have this be a const ref. Maybe pass a raw
Done
http://gerrit.cloudera.org:8080/#/c/16340/4/src/kudu/master/sys_catalog.cc@200
PS4, Line 200: masters specified in the on-disk Raft config, so log a warning instead of a strict check.
: // No further validations or populating 'last_kno
> nit: "Current" will change as the multi-master migration implementation con
Done
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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-Comment-Date: Wed, 02 Sep 2020 21:17:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc@57
PS1, Line 57: class MasterOptionsTest : public KuduTest {
: };
> Okay got it.
Yea those would be good validations to add.
My concern is less about whether new code will be compatible (since we can make it so :)), and more whether older versions of Kudu will fail to start, e.g. if operators failed tried to revert an upgrade.
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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: Sat, 15 Aug 2020 00:57:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has removed a vote on this change.
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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)
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16340
to look at the new patch set (#5).
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
[master] KUDU-3182 Allow single master to specify --master_addresses
'last_known_addr' field is not persisted for a single master Raft
configuration. This is okay as long as we have a single master
configuration. On dynamically transitioning from single master
to two master configuration, ChangeConfig() request to ADD_PEER
fails the validation in VerifyRaftConfig().
In order to transition to multi-master configuration, 'last_known_addr'
must be specified.
Some options considered were using the --rpc_bind_addresses flag, if
specified. If not, use GetHostName(). None of these are reliable.
So it's best to let user to specify the address of the single master
as is the case for specifying master address in tablet servers.
Currently --master_addresses flag must be empty for single master
configuration and --master_addresses with single address is not
permitted.
So the idea is to use existing --master_addresses flag for single
master configuration too and not just distributed configuration.
This change allows specifying --master_addresses flag for
single master configuration. To be backward compatible with existing
deployments, specifying this flag for single master configurations
is still optional and not required. If specified, it's used to set the
'last_known_addr' field in the master Raft configuration.
This change also provides mechanism for existing single master
Kudu deployments to specify the --master_addresses flag and populate
the 'last_known_addr' field in the master Raft configuration.
This change is a pre-requisite for upcoming dynamic multi-master
changes.
Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
A src/kudu/master/master_options-test.cc
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/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
14 files changed, 306 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/16340/5
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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)
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc@251
PS3, Line 251: Current
nit: do you expect this to change in the future? If not, can you remove "Current"? Same below.
It leaves me with the line of questioning, "If that was 'current' at time of writing, was it expected to change in the future? If so, did it change?"
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc@260
PS3, Line 260: if (peer.has_last_known_addr()) {
: // Verify the supplied master address matches with the on-disk Raft config.
: auto raft_master_addr = HostPortFromPB(peer.last_known_addr());
: if (raft_master_addr != master_addr) {
: return Status::InvalidArgument(
: Substitute("Single master Raft config error. On-disk master: $0 and "
: "supplied master: $1 are different", raft_master_addr.ToString(),
: master_addr.ToString()));
: }
Isn't it possible that of the on-disk peers, the first one isn't the one that matches this master's address?
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc@270
PS3, Line 270: // Set the Raft config for single master configuration.
: *config.mutable_peers(0)->mutable_last_known_addr() = HostPortToPB(master_addr);
Likewise, should this be setting the size of peers to 1?
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
Gerrit-PatchSet: 3
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-Comment-Date: Fri, 28 Aug 2020 23:35:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc@57
PS1, Line 57: class MasterOptionsTest : public KuduTest {
: };
> Yeah, even with the proposed change discussed in this comment, bringing up
That's not quite what I meant. I understand that starting up a new master with or without --master_addresses is allowed. My question is whether there are any issues with bringing up an older master with an empty --master_addresses, but with a single master already persisted (e.g. in the case of a downgrade -- not that we expect or support them).
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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: Sat, 15 Aug 2020 00:15:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16340/4/src/kudu/master/sys_catalog.h
File src/kudu/master/sys_catalog.h:
http://gerrit.cloudera.org:8080/#/c/16340/4/src/kudu/master/sys_catalog.h@360
PS4, Line 360: // For a single master config, verifies the on-disk Raft config and populates the
: // 'last_known_addr' in the Raft config, if master address is specified.
nit: Could you make it more obvious that this may update on-disk state?
http://gerrit.cloudera.org:8080/#/c/16340/4/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/16340/4/src/kudu/master/sys_catalog.cc@198
PS4, Line 198: const scoped_refptr<ConsensusMetadata>&
nit: it's a little misleading to have this be a const ref. Maybe pass a raw pointer and mention in the header comment that the pointer should outlive the call to this method?
http://gerrit.cloudera.org:8080/#/c/16340/4/src/kudu/master/sys_catalog.cc@200
PS4, Line 200: Current multi-master migration relies on starting up a single master with multiple
: // masters specified in the on-disk Raft config,
nit: "Current" will change as the multi-master migration implementation continues. Could we be more specific here, e.g.
"Manual single-to-multi-master migration relies on starting up a single master after updating the on-disk Raft config to contain multiple peers, so log a warning..." ?
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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-Comment-Date: Wed, 02 Sep 2020 19:55:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
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/16340
to look at the new patch set (#2).
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
[master] KUDU-3182 Allow single master to specify --master_addresses
'last_known_addr' field is not persisted for a single master Raft
configuration. This is okay as long as we have a single master
configuration. On dynamically transitioning from single master
to two master configuration, ChangeConfig() request to ADD_PEER
fails the validation in VerifyRaftConfig().
In order to transition to multi-master configuration, 'last_known_addr'
must be specified.
Some options considered were using the --rpc_bind_addresses flag, if
specified. If not, use GetHostName(). None of these are reliable.
So it's best to let user to specify the address of the single master
as is the case for specifying master address in tablet servers.
Currently --master_addresses flag must be empty for single master
configuration and --master_addresses with single address is not
permitted.
So the idea is to use existing --master_addresses flag for single
master configuration too and not just distributed configuration.
This change allows specifying --master_addresses flag for
single master configuration. To be backward compatible with existing
deployments, specifying this flag for single master configurations
is still optional and not required. If specified, it's used to set the
'last_known_addr' field in the master Raft configuration.
This change also provides mechanism for existing single master
Kudu deployments to specify the --master_addresses flag and populate
the 'last_known_addr' field in the master Raft configuration.
This change is a pre-requisite for upcoming dynamic multi-master
changes.
Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
A src/kudu/master/master_options-test.cc
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/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
13 files changed, 277 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/16340/2
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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-3182 Allow single master to specify --master addresses
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 5: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16340/5/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/16340/5/src/kudu/master/sys_catalog.cc@292
PS5, Line 292: } else {
: RETURN_NOT_OK(VerifyAndPopulateSingleMasterConfig(cmeta.get()));
readability nit: could you move this to be the first clause under 'if ()' ? I think it would help in code readability a bit.
http://gerrit.cloudera.org:8080/#/c/16340/5/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
http://gerrit.cloudera.org:8080/#/c/16340/5/src/kudu/util/net/net_util.h@107
PS5, Line 107: bool operator!=(const HostPort& hp1, const HostPort& hp2);
nit: I guess the definition of operator!= might be in-lined; or the definition of this method is put into the .cc file for some specific reason?
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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-Comment-Date: Wed, 02 Sep 2020 22:52:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16340
to look at the new patch set (#3).
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
[master] KUDU-3182 Allow single master to specify --master_addresses
'last_known_addr' field is not persisted for a single master Raft
configuration. This is okay as long as we have a single master
configuration. On dynamically transitioning from single master
to two master configuration, ChangeConfig() request to ADD_PEER
fails the validation in VerifyRaftConfig().
In order to transition to multi-master configuration, 'last_known_addr'
must be specified.
Some options considered were using the --rpc_bind_addresses flag, if
specified. If not, use GetHostName(). None of these are reliable.
So it's best to let user to specify the address of the single master
as is the case for specifying master address in tablet servers.
Currently --master_addresses flag must be empty for single master
configuration and --master_addresses with single address is not
permitted.
So the idea is to use existing --master_addresses flag for single
master configuration too and not just distributed configuration.
This change allows specifying --master_addresses flag for
single master configuration. To be backward compatible with existing
deployments, specifying this flag for single master configurations
is still optional and not required. If specified, it's used to set the
'last_known_addr' field in the master Raft configuration.
This change also provides mechanism for existing single master
Kudu deployments to specify the --master_addresses flag and populate
the 'last_known_addr' field in the master Raft configuration.
This change is a pre-requisite for upcoming dynamic multi-master
changes.
Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
A src/kudu/master/master_options-test.cc
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/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
13 files changed, 292 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/16340/3
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
Gerrit-PatchSet: 3
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)
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16340/5/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/16340/5/src/kudu/master/sys_catalog.cc@292
PS5, Line 292: return Status::InvalidArgument(msg);
: }
> readability nit: could you move this to be the first clause under 'if ()' ?
Done
http://gerrit.cloudera.org:8080/#/c/16340/5/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:
http://gerrit.cloudera.org:8080/#/c/16340/5/src/kudu/util/net/net_util.h@107
PS5, Line 107: inline bool operator!=(const HostPort& hp1, const HostPort& hp2) {
> nit: I guess the definition of operator!= might be in-lined; or the definit
Done
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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-Comment-Date: Thu, 03 Sep 2020 16:42:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16340/2/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/2/src/kudu/master/master_options-test.cc@181
PS2, Line 181: SetMasterAddresses
How about the case where there's a single master on disk and multiple supplied? I imagine that being the case for when a user wants to add a master. Will a single master currently fail?
http://gerrit.cloudera.org:8080/#/c/16340/2/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/16340/2/src/kudu/master/sys_catalog.cc@276
PS2, Line 276: RETURN_NOT_OK(consensus::VerifyRaftConfig(cstate.committed_config()));
Shouldn't we verify this before flushing to disk?
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
Gerrit-PatchSet: 2
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-Comment-Date: Fri, 28 Aug 2020 19:22:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
[master] KUDU-3182 Allow single master to specify --master_addresses
'last_known_addr' field is not persisted for a single master Raft
configuration. This is okay as long as we have a single master
configuration. On dynamically transitioning from single master
to two master configuration, ChangeConfig() request to ADD_PEER
fails the validation in VerifyRaftConfig().
In order to transition to multi-master configuration, 'last_known_addr'
must be specified.
Some options considered were using the --rpc_bind_addresses flag, if
specified. If not, use GetHostName(). None of these are reliable.
So it's best to let user to specify the address of the single master
as is the case for specifying master address in tablet servers.
Currently --master_addresses flag must be empty for single master
configuration and --master_addresses with single address is not
permitted.
So the idea is to use existing --master_addresses flag for single
master configuration too and not just distributed configuration.
This change allows specifying --master_addresses flag for
single master configuration. To be backward compatible with existing
deployments, specifying this flag for single master configurations
is still optional and not required. If specified, it's used to set the
'last_known_addr' field in the master Raft configuration.
This change also provides mechanism for existing single master
Kudu deployments to specify the --master_addresses flag and populate
the 'last_known_addr' field in the master Raft configuration.
This change is a pre-requisite for upcoming dynamic multi-master
changes.
Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Reviewed-on: http://gerrit.cloudera.org:8080/16340
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
A src/kudu/master/master_options-test.cc
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/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/util/net/net_util.h
13 files changed, 305 insertions(+), 18 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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)
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 4: Verified+1
> Patch Set 4: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/22324/ : FAILURE
Unrelated test failure in
MultiThreadedTabletTest/4.DeleteAndReinsert
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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-Comment-Date: Tue, 01 Sep 2020 21:54:39 +0000
Gerrit-HasComments: No
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc@57
PS1, Line 57: class MasterOptionsTest : public KuduTest {
: };
> One minor downside, I see, is that for every single master restart with --master_addresses field specified, Raft config will need to be checked for existence of the field.
Yeah, though I suppose we're already doing this checking anyway upon initial creation.
Another thing worth thinking about: if we by default start persisting a single last_known_addr field, will older versions of Kudu still be able to start up properly? Even if we pass in an empty --master_addresses field?
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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: Fri, 14 Aug 2020 23:10:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16340/2/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/2/src/kudu/master/master_options-test.cc@181
PS2, Line 181: SetMasterAddresses
> How about the case where there's a single master on disk and multiple suppl
Added a test case for the case you mention, though validation for that case is taken care by current code without this change.
As per current design, adding a master is currently initiated by RPC request.
http://gerrit.cloudera.org:8080/#/c/16340/2/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/16340/2/src/kudu/master/sys_catalog.cc@276
PS2, Line 276: RETURN_NOT_OK(consensus::VerifyRaftConfig(cstate.committed_config()));
> Shouldn't we verify this before flushing to disk?
Good point. Done.
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
Gerrit-PatchSet: 2
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-Comment-Date: Fri, 28 Aug 2020 20:49:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options-test.cc@84
PS3, Line 84: TestSingleMasterWithMasterAddresses
> The point of such validation would be to make sure user hasn't accidentally
Right, I was trying to understand what happens when the address specified with --master_addresses is not correct. In other words, whether it's possible to run with incorrect address and discover that only later on, and whether it would be hard to correct in that case.
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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-Comment-Date: Wed, 02 Sep 2020 22:26:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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-Comment-Date: Wed, 02 Sep 2020 21:56:38 +0000
Gerrit-HasComments: No
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 4:
> Patch Set 4: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/22323/ : FAILURE
Something weird is going on as the dist-test job timed out.
http://dist-test.cloudera.org/job?job_id=jenkins-slave.1598989285.19047
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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-Comment-Date: Tue, 01 Sep 2020 20:47:55 +0000
Gerrit-HasComments: No
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options-test.cc@84
PS3, Line 84: TestSingleMasterWithMasterAddresses
Does it make sense to add a scenario when a single-master cluster is given an address for master which master cannot bind to (e.g., IP address of another machine which isn't configured for the node's network interface)? What error do we expect to get in such case?
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options-test.cc@92
PS3, Line 92: string
nit: add 'const' to make it explicit that this isn't changing during the scenario?
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options.h
File src/kudu/master/master_options.h:
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options.h@39
PS3, Line 39: get
nit: output
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc@251
PS3, Line 251: Current
> nit: do you expect this to change in the future? If not, can you remove "Cu
Could you also specify what particular use case requires this? Is that something migrating from multi-master config to a single master?
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc@270
PS3, Line 270: // Set the Raft config for single master configuration.
BTW, what kind of use case this addresses?
What if this instance was running a multi-master there was multi-master config, but then it was started without --master_addresses flag by a mistake? Should we at least log a warning here?
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc@329
PS3, Line 329: Status s = master_->opts().GetTheOnlyMasterAddress(&hp);
: if (s.ok()) {
nit: if Status is not used beyond checking for OK/non-OK status, maybe shorten this into:
if (master_->opts().GetTheOnlyMasterAddress(&hp)) {
...
}
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
Gerrit-PatchSet: 3
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-Comment-Date: Mon, 31 Aug 2020 21:32:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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-Comment-Date: Thu, 03 Sep 2020 17:53:10 +0000
Gerrit-HasComments: No
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 1:
(1 comment)
> Patch Set 1:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc@57
PS1, Line 57: class MasterOptionsTest : public KuduTest {
: };
> > One minor downside, I see, is that for every single master restart with -
Yeah, even with the proposed change discussed in this comment, bringing up single master with empty --master_addresses is still okay.
Noted in the commit message.
"To be compatible with existing
deployments, specifying this flag for single master configurations
is still optional and not required. If specified, it's used
to set the 'last_known_addr' field in the master Raft configuration."
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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: Sat, 15 Aug 2020 00:10:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16340/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16340/1//COMMIT_MSG@15
PS1, Line 15: Inorder
nit: In order
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc@57
PS1, Line 57: class MasterOptionsTest : public KuduTest {
: };
Will existing masters be able to specify this flag and update their Raft configs?
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options.h
File src/kudu/master/master_options.h:
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options.h@40
PS1, Line 40: NOT_FOUND
nit: we usually refer to Status errors in TitleCase
Also may return IllegalState
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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: Fri, 14 Aug 2020 20:37:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16340
to look at the new patch set (#4).
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
[master] KUDU-3182 Allow single master to specify --master_addresses
'last_known_addr' field is not persisted for a single master Raft
configuration. This is okay as long as we have a single master
configuration. On dynamically transitioning from single master
to two master configuration, ChangeConfig() request to ADD_PEER
fails the validation in VerifyRaftConfig().
In order to transition to multi-master configuration, 'last_known_addr'
must be specified.
Some options considered were using the --rpc_bind_addresses flag, if
specified. If not, use GetHostName(). None of these are reliable.
So it's best to let user to specify the address of the single master
as is the case for specifying master address in tablet servers.
Currently --master_addresses flag must be empty for single master
configuration and --master_addresses with single address is not
permitted.
So the idea is to use existing --master_addresses flag for single
master configuration too and not just distributed configuration.
This change allows specifying --master_addresses flag for
single master configuration. To be backward compatible with existing
deployments, specifying this flag for single master configurations
is still optional and not required. If specified, it's used to set the
'last_known_addr' field in the master Raft configuration.
This change also provides mechanism for existing single master
Kudu deployments to specify the --master_addresses flag and populate
the 'last_known_addr' field in the master Raft configuration.
This change is a pre-requisite for upcoming dynamic multi-master
changes.
Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
A src/kudu/master/master_options-test.cc
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/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
14 files changed, 307 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/16340/4
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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)
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16340/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16340/1//COMMIT_MSG@15
PS1, Line 15: Inorder
> nit: In order
Done
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc@57
PS1, Line 57: class MasterOptionsTest : public KuduTest {
: };
> Yea those would be good validations to add.
Re: forward compatibility ie. older code reading Raft config persisted by newer code.
Most code I see checks for presence of 'last_known_addr' field and uses it if present.
So mostly looks okay.
One hypothetical case, after the master is downgraded and operator decides to change the hostname of the master. In that case there'll be discrepancy between the Raft persisted config and the hostname the master is running at. This is okay as long as the master remains in single master config. In older code, the Raft config will need to be manually updated to migrate to multiple masters.
If the master is subsequently upgraded and --master_addresses supplied is different then as per new validations in PS2, master will not come up. Manually updating the Raft config using CLI tool would be one option.
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options.h
File src/kudu/master/master_options.h:
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options.h@40
PS1, Line 40: NOT_FOUND
> nit: we usually refer to Status errors in TitleCase
Done
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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, 26 Aug 2020 19:18:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/1/src/kudu/master/master_options-test.cc@57
PS1, Line 57: class MasterOptionsTest : public KuduTest {
: };
> That's not quite what I meant. I understand that starting up a new master w
Okay got it.
I can add validations for such a case where on-disk config has a last_known_addr but --master_addresses is empty.
Also if a single address is specified in --master_addresses and on-disk config has a last_known_addr then they should match.
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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: Sat, 15 Aug 2020 00:40:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16340
to look at the new patch set (#6).
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
[master] KUDU-3182 Allow single master to specify --master_addresses
'last_known_addr' field is not persisted for a single master Raft
configuration. This is okay as long as we have a single master
configuration. On dynamically transitioning from single master
to two master configuration, ChangeConfig() request to ADD_PEER
fails the validation in VerifyRaftConfig().
In order to transition to multi-master configuration, 'last_known_addr'
must be specified.
Some options considered were using the --rpc_bind_addresses flag, if
specified. If not, use GetHostName(). None of these are reliable.
So it's best to let user to specify the address of the single master
as is the case for specifying master address in tablet servers.
Currently --master_addresses flag must be empty for single master
configuration and --master_addresses with single address is not
permitted.
So the idea is to use existing --master_addresses flag for single
master configuration too and not just distributed configuration.
This change allows specifying --master_addresses flag for
single master configuration. To be backward compatible with existing
deployments, specifying this flag for single master configurations
is still optional and not required. If specified, it's used to set the
'last_known_addr' field in the master Raft configuration.
This change also provides mechanism for existing single master
Kudu deployments to specify the --master_addresses flag and populate
the 'last_known_addr' field in the master Raft configuration.
This change is a pre-requisite for upcoming dynamic multi-master
changes.
Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
A src/kudu/master/master_options-test.cc
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/mini_master.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/util/net/net_util.h
13 files changed, 305 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/16340/6
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
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)
[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16340 )
Change subject: [master] KUDU-3182 Allow single master to specify --master_addresses
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options-test.cc
File src/kudu/master/master_options-test.cc:
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options-test.cc@84
PS3, Line 84: TestSingleMasterWithMasterAddresses
> Does it make sense to add a scenario when a single-master cluster is given
The point of such validation would be to make sure user hasn't accidentally supplied incorrect master address, right?
There is already --rpc_bind_addresses for RPC server which defaults to wildcard 0.0.0.0 and used to bind the RPC services.
For multi-masters with new deployments, --master_addresses is used to contact the supplied masters and fetch the permanent uuid. If incorrect master addresses were supplied then I imagine they'll become part of the distributed master config, if those master addresses are reachable with kudu master running on them.
Moreover given we already have --rpc_bind_addresses, not sure how to go about validating it?
We try to bind? Or check the hostname?
That brings up the original decision about not relying on the hostname/rpc_bind_addresses to populate the 'last_known_addr'.
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options-test.cc@92
PS3, Line 92: string
> nit: add 'const' to make it explicit that this isn't changing during the sc
Done
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options.h
File src/kudu/master/master_options.h:
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/master_options.h@39
PS3, Line 39: get
> nit: output
Done
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc@251
PS3, Line 251: Current
> Could you also specify what particular use case requires this? Is that som
This is for manually migrating from a single master to multi-master config using the current documented approach.
https://kudu.apache.org/docs/administration.html#migrate_to_multi_master
Tested by master_migration-itest
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc@260
PS3, Line 260: if (peer.has_last_known_addr()) {
: // Verify the supplied master address matches with the on-disk Raft config.
: auto raft_master_addr = HostPortFromPB(peer.last_known_addr());
: if (raft_master_addr != master_addr) {
: return Status::InvalidArgument(
: Substitute("Single master Raft config error. On-disk master: $0 and "
: "supplied master: $1 are different", raft_master_addr.ToString(),
: master_addr.ToString()));
: }
> Isn't it possible that of the on-disk peers, the first one isn't the one th
I've modified to code to skip validations and populating 'last_known_addr' field when on-disk Raft config has more than 1 master but only single master address is specified in --master_addresses flag.
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc@270
PS3, Line 270: // Set the Raft config for single master configuration.
: *config.mutable_peers(0)->mutable_last_known_addr() = HostPortToPB(master_addr);
> Likewise, should this be setting the size of peers to 1?
I've modified to code to skip validations and populating 'last_known_addr' field when on-disk Raft config has more than 1 master but only single master address is specified in --master_addresses flag.
http://gerrit.cloudera.org:8080/#/c/16340/3/src/kudu/master/sys_catalog.cc@329
PS3, Line 329: Status s = master_->opts().GetTheOnlyMasterAddress(&hp);
: if (s.ok()) {
> nit: if Status is not used beyond checking for OK/non-OK status, maybe shor
Done
--
To view, visit http://gerrit.cloudera.org:8080/16340
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4fe1bcd217d68f66db72c321397d596cba4224be
Gerrit-Change-Number: 16340
Gerrit-PatchSet: 3
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-Comment-Date: Tue, 01 Sep 2020 19:21:25 +0000
Gerrit-HasComments: Yes