Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13354 )
Change subject: [tool] Add cluster name resolver for CLI tools ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/13354/4/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/13354/4/src/kudu/tools/kudu-tool-test.cc@612 PS4, Line 612: void PrepareConfigFile(const string& content) { > If you ASSERT inside a function, you need to wrap calls to it with NO_FATAL Done http://gerrit.cloudera.org:8080/#/c/13354/4/src/kudu/tools/kudu-tool-test.cc@893 PS4, Line 893: int idx = 0; : ASSERT_OK(cluster_->GetLeaderMasterIndex(&idx)); : ASSERT_OK(CreateTabletServerMap(cluster_->master_proxy(idx), > Why was this change needed? CreateTabletServerMap sends the ListTabletServe There is an assert "CHECK_EQ(1, mini_masters_.size());" in master_proxy(), we can't call it when the mini cluster has more than one masters. Here, I can change it to cluster_->master_proxy(0). http://gerrit.cloudera.org:8080/#/c/13354/4/src/kudu/tools/kudu-tool-test.cc@4644 PS4, Line 4644: CHECK_ERR(unsetenv("KUDU_CONFIG")); > Shouldn't be necessary; there's no reason for this to be set in any test un No, this is to cleanup existing environment variable, e.g. my own developing platform. http://gerrit.cloudera.org:8080/#/c/13354/4/src/kudu/tools/kudu-tool-test.cc@4708 PS4, Line 4708: // Missing specified cluster name. : string content = Substitute( : R"*(clusters_info:)*""\n" : R"*( $0some_suffix:)*""\n" : R"*( master_addresses: $1)*", kClusterName, master_addrs_str); : PrepareConfigFile(content); : string stderr; : Status s = RunActionStderrString( : Substitute("master list @$0", kClusterName), : &stderr); : ASSERT_TRUE(s.IsRuntimeError()); : ASSERT_STR_CONTAINS( : stderr, Substitute("Not found: parse field $0 error: invalid node; ", : kClusterName)); > This is a fairly common pattern in this test (write out a config file, run Done http://gerrit.cloudera.org:8080/#/c/13354/4/src/kudu/tools/kudu-tool-test.cc@4764 PS4, Line 4764: auto master_addrs_str > You can call HostPort::ToCommaSeparatedString(cluster_->master_rpc_addrs()) Done http://gerrit.cloudera.org:8080/#/c/13354/4/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/13354/4/src/kudu/tools/tool_action_common.h@160 PS4, Line 160: // Parse Kudu master addresses into a string according to the 'master_addresses_arg' : // argument in 'context'. : // Treat master_addresses_arg as a cluster name if it is beginning with a '@', or : // treat it as master addresses. : // Content in ${KUDU_CONFIG}/kudurc looks like: : // clusters_info: : // cluster1: : // master_addresses: ip1:port1,ip2:port2,ip3:port3 : // cluster2: : // master_addresses: ip4:port4 > Nit: reword as: Done -- To view, visit http://gerrit.cloudera.org:8080/13354 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01 Gerrit-Change-Number: 13354 Gerrit-PatchSet: 4 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 28 May 2019 06:43:36 +0000 Gerrit-HasComments: Yes
