[kudu-CR] [master] KUDU-3182 Allow single master to specify --master addresses

["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