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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Wed, 22 May 2019 16:47:20 +0000 Gerrit-HasComments: Yes
