You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2019/05/16 17:13:49 UTC
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13354
Change subject: [tool] Add cluster name resolver for CLI tools
......................................................................
[tool] Add cluster name resolver for CLI tools
Kudu master rpc addresses are not easy to remember when use CLI
tools, even worse, when manage several Kudu clusters.
This patch add a YAML configuration file cluster_info.yaml in
${KUDU_HOME} for cluster name resolving, and there is a simple
example in that file.
Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
---
A cluster_info.yaml
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
10 files changed, 117 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/13354/1
--
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: newchange
Gerrit-Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
Gerrit-Change-Number: 13354
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Mitch Barnett, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13354
to look at the new patch set (#5).
Change subject: [tool] Add cluster name resolver for CLI tools
......................................................................
[tool] Add cluster name resolver for CLI tools
Kudu master rpc addresses are not easy to remember when use CLI
tools, even worse, when manage several Kudu clusters.
This patch add a feature to allow CLI tools to resolve cluster
name configured in a YAML file ${$KUDU_CONFIG}/cluster_info.yaml.
Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
9 files changed, 282 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/13354/5
--
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: newpatchset
Gerrit-Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
Gerrit-Change-Number: 13354
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Mitch Barnett, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13354
to look at the new patch set (#3).
Change subject: [tool] Add cluster name resolver for CLI tools
......................................................................
[tool] Add cluster name resolver for CLI tools
Kudu master rpc addresses are not easy to remember when use CLI
tools, even worse, when manage several Kudu clusters.
This patch add a feature to allow CLI tools to resolve cluster
name configured in a YAML file ${$KUDU_CONFIG}/cluster_info.yaml.
Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
9 files changed, 175 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/13354/3
--
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: newpatchset
Gerrit-Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
Gerrit-Change-Number: 13354
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Mitch Barnett, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13354
to look at the new patch set (#6).
Change subject: [tool] Add cluster name resolver for CLI tools
......................................................................
[tool] Add cluster name resolver for CLI tools
Kudu master rpc addresses are not easy to remember when use CLI
tools, even worse, when manage several Kudu clusters.
This patch add a feature to allow CLI tools to resolve cluster
name configured in a YAML file ${$KUDU_CONFIG}/cluster_info.yaml.
Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
9 files changed, 282 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/13354/6
--
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: newpatchset
Gerrit-Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
Gerrit-Change-Number: 13354
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13354 )
Change subject: [tool] Add cluster name resolver for CLI tools
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
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: deleteReviewer
Gerrit-Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
Gerrit-Change-Number: 13354
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 28 May 2019 03:24:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13354 )
Change subject: [tool] Add cluster name resolver for CLI tools
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc@503
PS1, Line 503: // master_addresses: ip1:port1,ip2:port2,ip3:port3
> Done
What do you think about using '@' or some other character that we know is invalid as a hostname to designate a cluster name? eg
kudu table list @mycluster
('@' seems like a decent choice because it's not a special shell character whereas the rest of !#$%^&*() all seem to have some special significance in shells and would need escaping)
--
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: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 22 May 2019 17:14:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc@503
PS1, Line 503: // master_addresses: ip1:port1,ip2:port2,ip3:port3
> What do you think about using '@' or some other character that we know is i
Good idea, I'll follow this
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc@198
PS2, Line 198: or a cluster name if it has "
: "been configured in $KUDU_HOME/cluster_info.yaml";
> Need to update this.
Done
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc@512
PS2, Line 512: auto config_file = string(kudu_config_path) + "/cluster_info.yaml";
> This doesn't adhere to the hierarchy Todd proposed in KUDU-1948. Why not go
I think as a environment variable, ${KUDU_CONFIG} is set according to the places and order '/etc/profile -> (~/.bash_profile | ~/.bash_login | ~/.profile) -> ~/.bashrc' in Linux, we don't have to invent another one. If we have to, means the existed Kudu environment variables like ${KUDU_HOIME} ${KUDU_USER_NAME} have move there too?
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc@513
PS2, Line 513: if (Env::Default()->FileExists(config_file)) {
> If KUDU_CONFIG is specified but the file doesn't exist, I think we can retu
Done
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc@515
PS2, Line 515: CHECK_OK(reader.Init());
> Can you convert these to RETURN_NOT_OK?
Done
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc@521
PS2, Line 521: LOG(INFO) << "Resolved cluster name according to " << config_file
: << ". Master addresses: " << master_addresses_str;
> Probably don't want an INFO-level log in a CLI tool.
Done
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_table.cc@504
PS2, Line 504: "Comma-separated list of Kudu Master addresses (source) "
: "where each address is of form 'hostname:port'" })
: .AddRequiredParameter({ kTableNameArg, "Name of the source table" })
: .AddRequiredParameter({ kDestMasterAddressesArg,
: "Comma-separated list of Kudu Master addresses (destination) "
: "where each address is of form 'hostname:port'" })
> These should be updated to reflect the cluster name thing too. Maybe scan t
Done, and I have checked all code now.
--
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: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 23 May 2019 07:16:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13354/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13354/1//COMMIT_MSG@11
PS1, Line 11: This patch add a YAML configuration file cluster_info.yaml in
: ${KUDU_HOME} for cluster name resolving, and there is a simple
: example in that file.
Hmm, do we really need this example file? It adds clutter to the top-level source directory, and it's not actually useful without modification, right? Perhaps we can store it in a more appropriate subdirectory? Or in a code comment somewhere?
http://gerrit.cloudera.org:8080/#/c/13354/1/cluster_info.yaml
File cluster_info.yaml:
PS1:
This file merits a copyright header.
http://gerrit.cloudera.org:8080/#/c/13354/1/cluster_info.yaml@3
PS1, Line 3: master_rpcs: ip1:port1,ip2:port2,ip3:port3
master_addresses would be a better name for this; it's more commonly found in our CLI args and tooling.
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc@503
PS1, Line 503: bool IsClusterName(const string& master_addresses_str) {
Why not just assume that any string can be a cluster name? If someone decides to name their cluster "master1,master2,master3" or "master1:12345", who are we to stop them? Or does YAML place restrictions on the allowed character set?
Basically, the flow could be:
1. Run the tool.
2. Look for a config file.
3. If found, parse it.
4. Compare the master addresses provided by the user to the cluster names in the config file.
5. If there's a match, extract the master addresses from the cluster name.
6. If not, treat the user's master addresses as verbatim addresses.
Errors in step #2 are non-fatal, but errors in step #3 and #5 are probably fatal (or at least deserving of a strong warning).
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc@512
PS1, Line 512: char* kudu_home = getenv("KUDU_HOME");
KUDU_HOME is only really useful in development contexts. How about the hierarchy of locations that Todd proposed in KUDU-1948?
1. Path given by $KUDUCONFIG
2. $HOME/.kudurc
3. /etc/kudu/kudurc
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc@513
PS1, Line 513: CHECK(kudu_home);
These CHECKs should be converted into bad Statuses. You'll probably need to adjust the function signature to pass back the parsed master addresses as an argument.
--
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: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 May 2019 18:29:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 5:
(3 comments)
BTW, now that this is wrapping up we should further document the layout and semantics of the YAML-based client config file. Would you like to do that in a follow-up patch?
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@893
PS4, Line 893: kTestCopyTableUpsert,
: kTestCopyTableSchemaOnly,
: kTestCopyTableComple
> There is an assert "CHECK_EQ(1, mini_masters_.size());" in master_proxy(),
Makes sense, thanks for the explanation.
http://gerrit.cloudera.org:8080/#/c/13354/4/src/kudu/tools/kudu-tool-test.cc@4644
PS4, Line 4644: ASSERT_TRUE(s.IsRuntimeError());
> No, this is to cleanup existing environment variable, e.g. my own developin
Yeah but that's unique to your environment, right? Why should it be checked into the test?
http://gerrit.cloudera.org:8080/#/c/13354/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/13354/5/src/kudu/tools/kudu-tool-test.cc@619
PS5, Line 619: Corrput
Corrupt
--
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: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 28 May 2019 18:26:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 3:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/kudu-tool-test.cc@4649
PS3, Line 4649: CHECK_ERR(setenv("KUDU_CONFIG", GetTestDataDirectory().c_str(), 1));
Should add some negative tests:
1. KUDU_CONFIG not defined but you use a cluster name anyway.
2. KUDU_CONFIG defined but there's no config file in it.
3. Config file is malformed or corrupt.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/kudu-tool-test.cc@4650
PS3, Line 4650: ostringstream buf;
: buf << "# some header comments" << endl;
: buf << kClusterName << ":" << " # some section comments" << endl;
: buf << " master_addresses: " << master_addrs_str << " # some key comments" << endl;
Maybe define this as a raw string literal wrapped by strings::Substitute? See https://en.cppreference.com/w/cpp/language/string_literal. May be clearer.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/kudu-tool-test.cc@4658
PS3, Line 4658: CHECK_OK(env_->NewWritableFile(fname, &writable_file));
Should use ASSERT_OK here and below.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/kudu-tool-test.cc@4673
PS3, Line 4673: CHECK_ERR(unsetenv("KUDU_CONFIG"));
Wrap this in a SCOPED_CLEANUP just after the setenv() call.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.h@159
PS3, Line 159: Status ParseMasterAddressesStr(
Please doc all of these.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@496
PS3, Line 496: bool IsClusterName(const string& master_addresses_str) {
Should be moved into an anonymous namespace or declared static.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@497
PS3, Line 497: return master_addresses_str.find('@') == 0;
gutil/strings/util.h defines HasPrefixString() which is more idiomatic.
Alternatively, you could incorporate the substr() from L517 into this function, like:
bool GetClusterName(const string& master_addresses_str, string* cluster_name);
It returns true if 'master_addresses_str' defines a cluster name, and if so, writes it to 'cluster_name'.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@500
PS3, Line 500: // Treat master_addresses_arg as a cluster name if it is beginning with a '@', or
Move this to the header file where it's more visible.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@520
PS3, Line 520: return Status::ConfigurationError("${KUDU_CONFIG} is missing");
How about "KUDU_CONFIG environment variable is missing"?
Should probably also use NotFound here instead; it's more typical for cases like these.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@522
PS3, Line 522: auto config_file = string(kudu_config_path) + "/cluster_info.yaml";
Would still prefer a different name for this file, like kudurc or something. Recall that the overarching goal of KUDU-1948 is to provide a generic client configuration file, which presumably may include configuration beyond clusters.
Also use JoinPathSegments to glue these two together.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@525
PS3, Line 525: Substitute("Configuration file cluster_info.yaml is missing in $0",
How about "Configuration file $0 was not found" where $0 is the path + cluster_info.yaml?
Should probably use NotFound here; it's more typical when files aren't found.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@539
PS3, Line 539: std::string* master_addresses_str) {
Got some unnecessary std:: prefixes here and below, and in a few other files too.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_table.cc@508
PS3, Line 508: destinationn
destination
But this won't look as good as before. Perhaps you can define a new Desc constant with (destination) infixed as before?
--
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: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 23 May 2019 07:52:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13354 )
Change subject: [tool] Add cluster name resolver for CLI tools
......................................................................
[tool] Add cluster name resolver for CLI tools
Kudu master rpc addresses are not easy to remember when use CLI
tools, even worse, when manage several Kudu clusters.
This patch add a feature to allow CLI tools to resolve cluster
name configured in a YAML file ${$KUDU_CONFIG}/cluster_info.yaml.
Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
Reviewed-on: http://gerrit.cloudera.org:8080/13354
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
9 files changed, 282 insertions(+), 53 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
Gerrit-Change-Number: 13354
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 28 May 2019 06:43:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Mitch Barnett, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13354
to look at the new patch set (#4).
Change subject: [tool] Add cluster name resolver for CLI tools
......................................................................
[tool] Add cluster name resolver for CLI tools
Kudu master rpc addresses are not easy to remember when use CLI
tools, even worse, when manage several Kudu clusters.
This patch add a feature to allow CLI tools to resolve cluster
name configured in a YAML file ${$KUDU_CONFIG}/cluster_info.yaml.
Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
9 files changed, 326 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/13354/4
--
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: newpatchset
Gerrit-Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
Gerrit-Change-Number: 13354
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 3:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/kudu-tool-test.cc@4649
PS3, Line 4649: CHECK_ERR(setenv("KUDU_CONFIG", GetTestDataDirectory().c_str(), 1));
> Should add some negative tests:
Done
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/kudu-tool-test.cc@4650
PS3, Line 4650: ostringstream buf;
: buf << "# some header comments" << endl;
: buf << kClusterName << ":" << " # some section comments" << endl;
: buf << " master_addresses: " << master_addrs_str << " # some key comments" << endl;
> Maybe define this as a raw string literal wrapped by strings::Substitute? S
Done
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/kudu-tool-test.cc@4658
PS3, Line 4658: CHECK_OK(env_->NewWritableFile(fname, &writable_file));
> Should use ASSERT_OK here and below.
Done
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/kudu-tool-test.cc@4673
PS3, Line 4673: CHECK_ERR(unsetenv("KUDU_CONFIG"));
> Wrap this in a SCOPED_CLEANUP just after the setenv() call.
Done
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.h@159
PS3, Line 159: Status ParseMasterAddressesStr(
> Please doc all of these.
Done
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@496
PS3, Line 496: bool IsClusterName(const string& master_addresses_str) {
> Should be moved into an anonymous namespace or declared static.
Done
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@497
PS3, Line 497: return master_addresses_str.find('@') == 0;
> gutil/strings/util.h defines HasPrefixString() which is more idiomatic.
Done
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@500
PS3, Line 500: // Treat master_addresses_arg as a cluster name if it is beginning with a '@', or
> Move this to the header file where it's more visible.
Done
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@520
PS3, Line 520: return Status::ConfigurationError("${KUDU_CONFIG} is missing");
> How about "KUDU_CONFIG environment variable is missing"?
kudu_config_path is NULL when it's missing.
And also, when the file is missing, return NotFound too, I'll fix it.
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@522
PS3, Line 522: auto config_file = string(kudu_config_path) + "/cluster_info.yaml";
> Would still prefer a different name for this file, like kudurc or something
Done
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@525
PS3, Line 525: Substitute("Configuration file cluster_info.yaml is missing in $0",
> How about "Configuration file $0 was not found" where $0 is the path + clus
Done
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_common.cc@539
PS3, Line 539: std::string* master_addresses_str) {
> Got some unnecessary std:: prefixes here and below, and in a few other file
Done
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/13354/3/src/kudu/tools/tool_action_table.cc@508
PS3, Line 508: destinationn
> destination
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: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Mon, 27 May 2019 07:30:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 5:
(2 comments)
> Patch Set 5:
>
> (3 comments)
>
> BTW, now that this is wrapping up we should further document the layout and semantics of the YAML-based client config file. Would you like to do that in a follow-up patch?
I will do that
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@4644
PS4, Line 4644: ASSERT_TRUE(s.IsRuntimeError());
> Yeah but that's unique to your environment, right? Why should it be checked
unsetenv returns success even if the environment variable doesn't exist, like in the Jenkins this test succeed. https://linux.die.net/man/3/unsetenv
However, in the environment which has this environment variable, this test will fail if not unsetenv, because this test is for the case that the environment variable is not set.
And I think after this patch has been merged, developers may add this ${KUDU_CONFIG} to their own developing environment, then this test will fail if not unsetenv.
http://gerrit.cloudera.org:8080/#/c/13354/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/13354/5/src/kudu/tools/kudu-tool-test.cc@619
PS5, Line 619: Corrput
> Corrupt
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: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 29 May 2019 02:03:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13354/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13354/1//COMMIT_MSG@11
PS1, Line 11: This patch add a feature to allow CLI tools to resolve cluster
: name configured in a YAML file ${$KUDU_CONFIG}/cluster_info.yaml.
:
> Hmm, do we really need this example file? It adds clutter to the top-level
OK, I'll remove it and add some comments to introduce how to use it.
http://gerrit.cloudera.org:8080/#/c/13354/1/cluster_info.yaml
File cluster_info.yaml:
PS1:
> This file merits a copyright header.
Done
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc@503
PS1, Line 503: // master_addresses: ip1:port1,ip2:port2,ip3:port3
> Why not just assume that any string can be a cluster name? If someone decid
Done
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc@512
PS1, Line 512: auto config_file = string(kudu_config_path) + "/cluster_info.yaml";
> KUDU_HOME is only really useful in development contexts. How about the hier
Done
http://gerrit.cloudera.org:8080/#/c/13354/1/src/kudu/tools/tool_action_common.cc@513
PS1, Line 513: if (Env::Default()->FileExists(config_file)) {
> These CHECKs should be converted into bad Statuses. You'll probably need to
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: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sat, 18 May 2019 03:46:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Mitch Barnett, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13354
to look at the new patch set (#2).
Change subject: [tool] Add cluster name resolver for CLI tools
......................................................................
[tool] Add cluster name resolver for CLI tools
Kudu master rpc addresses are not easy to remember when use CLI
tools, even worse, when manage several Kudu clusters.
This patch add a feature to allow CLI tools to resolve cluster
name configured in a YAML file ${$KUDU_CONFIG}/cluster_info.yaml.
Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
M src/kudu/tools/tool_action_tablet.cc
9 files changed, 117 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/13354/2
--
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: newpatchset
Gerrit-Change-Id: I2fc235906adbf25fcc2059fa8443e3013830ec01
Gerrit-Change-Number: 13354
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 6: Verified+1 Code-Review+2
(1 comment)
Overriding Jenkins, filed KUDU-2831 for the (unrelated) test failure.
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@4644
PS4, Line 4644: ASSERT_TRUE(s.IsRuntimeError());
> unsetenv returns success even if the environment variable doesn't exist, li
OK, I guess that's a fair point.
--
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: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 29 May 2019 16:36:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tool] Add cluster name resolver for CLI tools
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 2:
(6 comments)
Sorry I rereviewed this a few days ago and forgot to post my comments.
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc@198
PS2, Line 198: or a cluster name if it has "
: "been configured in $KUDU_HOME/cluster_info.yaml";
Need to update this.
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc@512
PS2, Line 512: auto config_file = string(kudu_config_path) + "/cluster_info.yaml";
This doesn't adhere to the hierarchy Todd proposed in KUDU-1948. Why not go with that, both in terms of the number of places to check as well as the name of the config file?
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc@513
PS2, Line 513: if (Env::Default()->FileExists(config_file)) {
If KUDU_CONFIG is specified but the file doesn't exist, I think we can return an error. It's a sign of a misconfiguration that we should surface to the user of the CLI.
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc@515
PS2, Line 515: CHECK_OK(reader.Init());
Can you convert these to RETURN_NOT_OK?
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_common.cc@521
PS2, Line 521: LOG(INFO) << "Resolved cluster name according to " << config_file
: << ". Master addresses: " << master_addresses_str;
Probably don't want an INFO-level log in a CLI tool.
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/13354/2/src/kudu/tools/tool_action_table.cc@504
PS2, Line 504: "Comma-separated list of Kudu Master addresses (source) "
: "where each address is of form 'hostname:port'" })
: .AddRequiredParameter({ kTableNameArg, "Name of the source table" })
: .AddRequiredParameter({ kDestMasterAddressesArg,
: "Comma-separated list of Kudu Master addresses (destination) "
: "where each address is of form 'hostname:port'" })
These should be updated to reflect the cluster name thing too. Maybe scan through the rest of the tool to see if there are other places?
--
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: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mitch Barnett <mb...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 22 May 2019 16:47:20 +0000
Gerrit-HasComments: Yes