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 <[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:52:29 +0000 Gerrit-HasComments: Yes
