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 <[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: Thu, 23 May 2019 07:16:31 +0000 Gerrit-HasComments: Yes
