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 <[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: Mon, 27 May 2019 07:30:27 +0000 Gerrit-HasComments: Yes
