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 <[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: Mon, 31 Aug 2020 21:32:44 +0000 Gerrit-HasComments: Yes
