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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett <[email protected]> Gerrit-Comment-Date: Fri, 17 May 2019 18:29:37 +0000 Gerrit-HasComments: Yes
