Adar Dembo 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) Looks good; only a few more nits. 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_FATALS(). Otherwise an ASSERT that fires will only be detected at the end of the test. In and of itself that's OK (at least the failure was detected at all!) but it means the rest of the test might fail in an unexpected way, which makes troubleshooting difficult. 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 ListTabletServers RPC through the provided proxy, but tservers send heartbeats to all masters, not just the leader master, so ts_map_ should be the same regardless of which master received the RPC. 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 unless we explicitly called setenv(). 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 the tool, assert on stderr). Maybe avoid repeating so much code with a lambda that takes the config file and expected stderr as input? 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())) instead. 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: // Parses 'master_addresses_arg' from 'context' into 'master_addresses_str', a // comma-separated string of host/port pairs. // // If 'master_addresses_arg' starts with a '@' it is interpreted as a cluster name and // resolved against a config file in ${KUDU_CONFIG}/kudurc with content like: // // clusters_info: // cluster1: // master_addresses: ip1:port1,ip2:port2,ip3:port3 // cluster2: -- 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 03:24:48 +0000 Gerrit-HasComments: Yes
