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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 01 Sep 2020 19:21:25 +0000 Gerrit-HasComments: Yes
