Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17995 )
Change subject: [master] KUDU-3311 Allow to start with diff num of masters ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/17995/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17995/5//COMMIT_MSG@12 PS5, Line 12: discouraging decommissioning masters : this way. This is also discourages unintentional migration, in which case the diff would still be 2 (-1 master, +1 master), right? http://gerrit.cloudera.org:8080/#/c/17995/5//COMMIT_MSG@15 PS5, Line 15: sys_catalog_load_cmeta-itest.cc nit: this is no longer the case, please update it to reflect the most recent state of the patch http://gerrit.cloudera.org:8080/#/c/17995/5/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/17995/5/src/kudu/master/dynamic_multi_master-test.cc@1477 PS5, Line 1477: TEST_F(AutoAddMasterTest, TestRestartMastersWhileSomeDown) { Consider adding another test case to AutoAddMasterTest that, instead of using ExternalMiniCluster::AddMaster(), configures the other masters with the new master address first, starts up, waits a bit to ensure things don't fail, and then creates a new ExternalMaster using CreateMaster() as we do in AddMaster() That way we test the more end-to-end behavior of actually being able to add the master after "misconfiguring" the old masters http://gerrit.cloudera.org:8080/#/c/17995/5/src/kudu/master/master_options-test.cc File src/kudu/master/master_options-test.cc: http://gerrit.cloudera.org:8080/#/c/17995/5/src/kudu/master/master_options-test.cc@223 PS5, Line 223: }); style-nit: we typically align end braces with the line that introduced it. In this case that might look like cluster.mini_master(0)->SetMasterAddresses({ master_addresses[0], master_addresses[1], HostPort("master-2", Master::kDefaultPort) }); http://gerrit.cloudera.org:8080/#/c/17995/5/src/kudu/master/master_options-test.cc@226 PS5, Line 226: || s.IsInvalidArgument()); nit: it isn't clear upon reading this why we're expecting an invalid argument here. Mind adding a comment? http://gerrit.cloudera.org:8080/#/c/17995/5/src/kudu/master/master_options-test.cc@243 PS5, Line 243: }); nit: same here w.r.t. alignment. Also consider spacing like at L219 just to keep things a bit more consistent -- To view, visit http://gerrit.cloudera.org:8080/17995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39aeee2f52a55a8c29770f748895d38c9adff8a2 Gerrit-Change-Number: 17995 Gerrit-PatchSet: 5 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 08 Nov 2021 22:51:23 +0000 Gerrit-HasComments: Yes
