Alexey Serbin 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 1: (14 comments) http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/integration-tests/sys_catalog_load_cmeta-itest.cc File src/kudu/integration-tests/sys_catalog_load_cmeta-itest.cc: http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/integration-tests/sys_catalog_load_cmeta-itest.cc@52 PS1, Line 52: void StartCluster() { : cluster_.reset(new ExternalMiniCluster(cluster_opts_)); : ASSERT_OK(cluster_->Start()); : } Remove this method and inline the code into SetUp() since this method isn't used anywhere else. http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/integration-tests/sys_catalog_load_cmeta-itest.cc@68 PS1, Line 68: vector<ExternalDaemon*> daemons = cluster_->daemons(); : for (ExternalDaemon* daemon : daemons) { : if (!daemon->IsShutdown()) { : return Status::IllegalState("Cluster did not shut down"); : } : } Is there any specific reason to check for this? If not, consider removing this extra checks: we have separate test cases for the extrnal mini-cluster that verify the functionality of the ExternalMiniCluster::Shutdown() method. http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/integration-tests/sys_catalog_load_cmeta-itest.cc@78 PS1, Line 78: RETURN_NOT_OK(cluster_->AddMaster()); : return Status::OK(); readability nit: could be written simply as return cluster_->AddMaster(); http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/integration-tests/sys_catalog_load_cmeta-itest.cc@84 PS1, Line 84: if (!master_hostports.empty()) { : RETURN_NOT_OK(cluster_->RemoveMaster(master_hostports[0])); : } else { : return Status::IllegalState("No masters to remove."); : } : return Status::OK(); readability nit: it would be a bit easier to read if it were written as if (master_hostports.empty()) { return Status::IllegalState("no masters to remove"): } return cluster_->RemoveMaster(master_hostports[0]); http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/integration-tests/sys_catalog_load_cmeta-itest.cc@93 PS1, Line 93: const int num_masters_; : const int num_tservers_; Are there member fields used anywhere at all? If not, consider removing them. http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/integration-tests/sys_catalog_load_cmeta-itest.cc@113 PS1, Line 113: Timed out waiting Here and in other places in this file: is there something particular to the error happened? The line above (ASSERT_TRUE(s.IsTimedOut())) already checks for the timed-out status, so maybe there is something about the master which failed to start up or something? http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/integration-tests/sys_catalog_load_cmeta-itest.cc@121 PS1, Line 121: ; nit for here and similar places in this file: if this assertion ever triggered, it would be easier to troubleshoot if printing out the actual information of the result status: ASSERT_TRUE(s.IsTimedOut()) << s.ToString(); http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/integration-tests/sys_catalog_load_cmeta-itest.cc@121 PS1, Line 121: nit: remove the extra space http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/master/sys_catalog.cc@295 PS1, Line 295: string msg; nit: remove the variable from this outer scope since it's not used there http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/master/sys_catalog.cc@296 PS1, Line 296: // We should prevent starting with fewer masters than previously. This way if the user wants to : // remove a master they would have to do it manually Just to double-check: you verified this works in scenarios involving the kudu CLI tool `kudu master remove`, right? One of such scenarios is described at https://kudu.apache.org/docs/administration.html#_removing_kudu_masters_from_a_multi_master_deployment http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/master/sys_catalog.cc@299 PS1, Line 299: msg nit for here and below: move Substitute() under the Status::InvalidArgument() -- a separate variable isn't needed http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/master/sys_catalog.cc@307 PS1, Line 307: } else if (symm_diff.size() > 1) { > warning: do not use 'else' after 'return' [readability-else-after-return] +1 http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/master/sys_catalog.cc@309 PS1, Line 309: difference is higher than 1. nit: maybe, replace with differ by more than one address http://gerrit.cloudera.org:8080/#/c/17995/1/src/kudu/master/sys_catalog.cc@317 PS1, Line 317: ( nit: why to add parentheses? Would it be OK to output the extra address right after the colon as is? -- 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: 1 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: Wed, 03 Nov 2021 22:46:49 +0000 Gerrit-HasComments: Yes
