kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From t...@apache.org
Subject [2/5] kudu git commit: tool: better handling for positional arguments
Date Mon, 22 Aug 2016 07:34:59 GMT
tool: better handling for positional arguments

This patch modifies Action to describe all of its argument requirements, not
just gflags. As a result, we can improve usability in two major ways:
1. The parser itself can process positional arguments, leaving less work for
   individual actions.
2. The action help strings can describe positional arguments.

Change-Id: Ic6aa517ccf1915e4d106bcc5f80a3abfeae03271
Reviewed-on: http://gerrit.cloudera.org:8080/4013
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <todd@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/00dd9fde
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/00dd9fde
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/00dd9fde

Branch: refs/heads/master
Commit: 00dd9fde71f6f8afd52e3e4a953b9eae331fb543
Parents: 1bc9f80
Author: Adar Dembo <adar@cloudera.com>
Authored: Tue Aug 16 19:35:44 2016 -0700
Committer: Todd Lipcon <todd@apache.org>
Committed: Fri Aug 19 18:48:20 2016 +0000

----------------------------------------------------------------------
 src/kudu/tools/tool_action.cc        |  97 +++++++++++++++++-------
 src/kudu/tools/tool_action.h         | 121 ++++++++++++++++++++----------
 src/kudu/tools/tool_action_fs.cc     |  29 +++----
 src/kudu/tools/tool_action_tablet.cc |  63 +++++++---------
 src/kudu/tools/tool_main.cc          |  56 +++++++++++++-
 src/kudu/util/subprocess.cc          |   4 +-
 6 files changed, 241 insertions(+), 129 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/00dd9fde/src/kudu/tools/tool_action.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tools/tool_action.cc b/src/kudu/tools/tool_action.cc
index 317c162..447c392 100644
--- a/src/kudu/tools/tool_action.cc
+++ b/src/kudu/tools/tool_action.cc
@@ -17,17 +17,17 @@
 
 #include "kudu/tools/tool_action.h"
 
-#include <deque>
 #include <memory>
 #include <string>
+#include <unordered_map>
 #include <vector>
 
 #include "kudu/gutil/strings/join.h"
 #include "kudu/gutil/strings/substitute.h"
 
-using std::deque;
 using std::string;
 using std::unique_ptr;
+using std::unordered_map;
 using std::vector;
 using strings::Substitute;
 
@@ -36,6 +36,26 @@ namespace tools {
 
 namespace {
 
+string FakeDescribeOneFlag(const ActionArgsDescriptor::Arg& arg) {
+  string res = google::DescribeOneFlag({
+    arg.name,        // name
+    "string",        // type
+    arg.description, // description
+    "",              // current_value
+    "",              // default_value
+    "",              // filename
+    false,           // has_validator_fn
+    true,            // is_default
+    nullptr          // flag_ptr
+  });
+
+  // Strip the first dash from the description; this is a positional parameter
+  // so let's make sure it looks like one.
+  string::size_type first_dash_idx = res.find("-");
+  DCHECK_NE(string::npos, first_dash_idx);
+  return res.substr(0, first_dash_idx) + res.substr(first_dash_idx + 1);
+}
+
 string BuildUsageString(const vector<Mode*>& chain) {
   string modes = JoinMapped(chain, [](Mode* a){ return a->name(); }, " ");
   return Substitute("Usage: $0", modes);
@@ -86,8 +106,26 @@ ActionBuilder::ActionBuilder(const Label& label, const ActionRunner&
runner)
       runner_(runner) {
 }
 
-ActionBuilder& ActionBuilder::AddGflag(const string& gflag) {
-  gflags_.push_back(gflag);
+ActionBuilder& ActionBuilder::AddRequiredParameter(
+    const ActionArgsDescriptor::Arg& arg) {
+  args_.required.push_back(arg);
+  return *this;
+}
+
+ActionBuilder& ActionBuilder::AddRequiredVariadicParameter(
+    const ActionArgsDescriptor::Arg& arg) {
+  DCHECK(!args_.variadic);
+  args_.variadic = arg;
+  return *this;
+}
+
+ActionBuilder& ActionBuilder::AddOptionalParameter(const string& param) {
+#ifndef NDEBUG
+  // Make sure this gflag exists.
+  string option;
+  DCHECK(google::GetCommandLineOption(param.c_str(), &option));
+#endif
+  args_.optional.push_back(param);
   return *this;
 }
 
@@ -95,50 +133,51 @@ unique_ptr<Action> ActionBuilder::Build() {
   unique_ptr<Action> action(new Action());
   action->label_ = label_;
   action->runner_ = runner_;
-  action->gflags_ = gflags_;
+  action->args_ = args_;
   return action;
 }
 
 Action::Action() {
 }
 
-Status Action::Run(const vector<Mode*>& chain, deque<string> args) const
{
-  return runner_(chain, this, args);
+Status Action::Run(const vector<Mode*>& chain,
+                   const unordered_map<string, string>& required_args,
+                   const vector<string>& variadic_args) const {
+  return runner_({ chain, this, required_args, variadic_args });
 }
 
 string Action::BuildHelp(const vector<Mode*>& chain) const {
-  string msg = Substitute("$0 $1", BuildUsageString(chain), name());
-  string gflags_msg;
-  for (const auto& gflag : gflags_) {
+  string usage_msg = Substitute("$0 $1", BuildUsageString(chain), name());
+  string desc_msg;
+  for (const auto& param : args_.required) {
+    usage_msg += Substitute(" <$0>", param.name);
+    desc_msg += FakeDescribeOneFlag(param);
+  }
+  if (args_.variadic) {
+    const ActionArgsDescriptor::Arg& param = args_.variadic.get();
+    usage_msg += Substitute(" <$0>...", param.name);
+    desc_msg += FakeDescribeOneFlag(param);
+  }
+  for (const auto& param : args_.optional) {
     google::CommandLineFlagInfo gflag_info =
-        google::GetCommandLineFlagInfoOrDie(gflag.c_str());
+        google::GetCommandLineFlagInfoOrDie(param.c_str());
     string noun;
-    int last_underscore_idx = gflag.rfind('_');
+    string::size_type last_underscore_idx = param.rfind('_');
     if (last_underscore_idx != string::npos &&
-        last_underscore_idx != gflag.size() - 1) {
-      noun = gflag.substr(last_underscore_idx + 1);
+        last_underscore_idx != param.size() - 1) {
+      noun = param.substr(last_underscore_idx + 1);
     } else {
-      noun = gflag;
+      noun = param;
     }
-    msg += Substitute(" [-$0=<$1>]", gflag, noun);
-    gflags_msg += google::DescribeOneFlag(gflag_info);
+    usage_msg += Substitute(" [-$0=<$1>]", param, noun);
+    desc_msg += google::DescribeOneFlag(gflag_info);
   }
+  string msg = usage_msg;
   msg += "\n";
   msg += Substitute("$0\n", label_.description);
-  msg += gflags_msg;
+  msg += desc_msg;
   return msg;
 }
 
-Status ParseAndRemoveArg(const char* arg_name,
-                         deque<string>* remaining_args,
-                         string* parsed_arg) {
-  if (remaining_args->empty()) {
-    return Status::InvalidArgument(Substitute("must provide $0", arg_name));
-  }
-  *parsed_arg = remaining_args->front();
-  remaining_args->pop_front();
-  return Status::OK();
-}
-
 } // namespace tools
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/00dd9fde/src/kudu/tools/tool_action.h
----------------------------------------------------------------------
diff --git a/src/kudu/tools/tool_action.h b/src/kudu/tools/tool_action.h
index cea7c7e..ec83212 100644
--- a/src/kudu/tools/tool_action.h
+++ b/src/kudu/tools/tool_action.h
@@ -17,14 +17,13 @@
 
 #pragma once
 
-#include <deque>
-#include <glog/logging.h>
 #include <memory>
 #include <string>
+#include <unordered_map>
 #include <vector>
 
-#include "kudu/gutil/strings/join.h"
-#include "kudu/gutil/strings/substitute.h"
+#include <boost/optional/optional.hpp>
+
 #include "kudu/util/status.h"
 
 namespace kudu {
@@ -126,12 +125,51 @@ class Mode {
   std::vector<std::unique_ptr<Action>> actions_;
 };
 
-// Function signature for any operation represented by an action. When run, the
-// operation receives the parent mode chain, the current action, and any
-// remaining command line arguments.
-typedef std::function<Status(const std::vector<Mode*>&,
-                             const Action*,
-                             std::deque<std::string>)> ActionRunner;
+// Function signature for any operation represented by an Action.
+//
+// When run, the operation receives the parent mode chain, the current action,
+// and the command line arguments.
+//
+// Prior to running, arguments registered by the action are put into the
+// context. In the event that a required parameter is missing or there are
+// unexpected arguments on the command line, an error is returned and the
+// runner will not be invoked.
+//
+// Note: only required and variadic args are put in the context; it is expected
+// that operations access optional args via gflag variables (i.e. FLAGS_foo).
+struct RunnerContext {
+  std::vector<Mode*> chain;
+  const Action* action;
+  std::unordered_map<std::string, std::string> required_args;
+  std::vector<std::string> variadic_args;
+};
+typedef std::function<Status(const RunnerContext&)> ActionRunner;
+
+// Describes all of the arguments used by an action. At runtime, the tool will
+// parse these arguments out of the command line and marshal them into a
+// key/value argument map (see Run()).
+struct ActionArgsDescriptor {
+  struct Arg {
+    std::string name;
+    std::string description;
+  };
+
+  // Positional (required) command line arguments.
+  std::vector<Arg> required;
+
+  // Key-value command line arguments. These must actually implemented as
+  // gflags, which means all that must be specified here are the gflag names.
+  // The gflag definitions themselves will be accessed to get the argument
+  // descriptions.
+  //
+  // Optional by definition, though some are required internally
+  // (e.g. fs_wal_dir).
+  std::vector<std::string> optional;
+
+  // Variable length command line argument. There may be at most one per
+  // Action, and it's always found at the end of the command line.
+  boost::optional<Arg> variadic;
+};
 
 // Builds a new action (leaf) node.
 class ActionBuilder {
@@ -139,8 +177,32 @@ class ActionBuilder {
   // Creates a new ActionBuilder with a specific label and action runner.
   ActionBuilder(const Label& label, const ActionRunner& runner);
 
-  // Add a new gflag to this builder. They are used when generating help.
-  ActionBuilder& AddGflag(const std::string& gflag);
+  // Add a new required parameter to this builder.
+  //
+  // This parameter will be parsed as a positional argument following the name
+  // of the action. The order in which required parameters are added to the
+  // builder reflects the order they are expected to be parsed from the command
+  // line.
+  ActionBuilder& AddRequiredParameter(
+      const ActionArgsDescriptor::Arg& arg);
+
+  // Add a new required variable-length parameter to this builder.
+  //
+  // This parameter will be parsed following all other positional parameters.
+  // All remaining positional arguments on the command line will be parsed into
+  // this parameter.
+  //
+  // There may be at most one variadic parameter defined per action.
+  ActionBuilder& AddRequiredVariadicParameter(
+      const ActionArgsDescriptor::Arg& arg);
+
+  // Add a new optional parameter to this builder.
+  //
+  // This parameter will be parsed by the gflags system, and thus can be
+  // provided by the user at any point in the command line. It must match a
+  // previously-defined gflag; if a gflag with the same name cannot be found,
+  // the tool will crash.
+  ActionBuilder& AddOptionalParameter(const std::string& param);
 
   // Creates an action using builder state.
   std::unique_ptr<Action> Build();
@@ -150,7 +212,7 @@ class ActionBuilder {
 
   ActionRunner runner_;
 
-  std::vector<std::string> gflags_;
+  ActionArgsDescriptor args_;
 };
 
 // A leaf node in the tree, representing a logical operation taken by the tool.
@@ -161,13 +223,17 @@ class Action {
   std::string BuildHelp(const std::vector<Mode*>& chain) const;
 
   // Runs the operation represented by this action, given a parent mode chain
-  // and list of extra command line arguments.
-  Status Run(const std::vector<Mode*>& chain, std::deque<std::string> args)
const;
+  // and marshaled command line arguments.
+  Status Run(const std::vector<Mode*>& chain,
+             const std::unordered_map<std::string, std::string>& required_args,
+             const std::vector<std::string>& variadic_args) const;
 
   const std::string& name() const { return label_.name; }
 
   const std::string& description() const { return label_.description; }
 
+  const ActionArgsDescriptor& args() const { return args_; }
+
  private:
   friend class ActionBuilder;
 
@@ -177,32 +243,9 @@ class Action {
 
   ActionRunner runner_;
 
-  // This action's gflags (if any).
-  std::vector<std::string> gflags_;
+  ActionArgsDescriptor args_;
 };
 
-// Removes one argument from 'remaining_args' and stores it in 'parsed_arg'.
-//
-// If 'remaining_args' is empty, returns InvalidArgument with 'arg_name' in the
-// message.
-Status ParseAndRemoveArg(const char* arg_name,
-                         std::deque<std::string>* remaining_args,
-                         std::string* parsed_arg);
-
-// Checks that 'args' is empty. If not, returns a bad status.
-template <typename CONTAINER>
-Status CheckNoMoreArgs(const std::vector<Mode*>& chain,
-                       const Action* action,
-                       const CONTAINER& args) {
-  if (args.empty()) {
-    return Status::OK();
-  }
-  DCHECK(!chain.empty());
-  return Status::InvalidArgument(
-      strings::Substitute("too many arguments: '$0'\n$1",
-                          JoinStrings(args, " "), action->BuildHelp(chain)));
-}
-
 // Returns a new "fs" mode node.
 std::unique_ptr<Mode> BuildFsMode();
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/00dd9fde/src/kudu/tools/tool_action_fs.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tools/tool_action_fs.cc b/src/kudu/tools/tool_action_fs.cc
index a77bc06..3ed3efa 100644
--- a/src/kudu/tools/tool_action_fs.cc
+++ b/src/kudu/tools/tool_action_fs.cc
@@ -17,18 +17,17 @@
 
 #include "kudu/tools/tool_action.h"
 
-#include <boost/optional/optional.hpp>
-#include <deque>
-#include <gflags/gflags.h>
 #include <iostream>
 #include <memory>
 #include <string>
 
+#include <boost/optional/optional.hpp>
+#include <gflags/gflags.h>
+
 #include "kudu/fs/fs_manager.h"
 #include "kudu/util/status.h"
 
 using std::cout;
-using std::deque;
 using std::endl;
 using std::string;
 using std::unique_ptr;
@@ -42,11 +41,7 @@ namespace tools {
 
 namespace {
 
-Status Format(const vector<Mode*>& chain,
-              const Action* action,
-              deque<string> args) {
-  RETURN_NOT_OK(CheckNoMoreArgs(chain, action, args));
-
+Status Format(const RunnerContext& context) {
   FsManager fs_manager(Env::Default(), FsManagerOpts());
   boost::optional<string> uuid;
   if (!FLAGS_uuid.empty()) {
@@ -55,11 +50,7 @@ Status Format(const vector<Mode*>& chain,
   return fs_manager.CreateInitialFileSystemLayout(uuid);
 }
 
-Status PrintUuid(const vector<Mode*>& chain,
-                 const Action* action,
-                 deque<string> args) {
-  RETURN_NOT_OK(CheckNoMoreArgs(chain, action, args));
-
+Status PrintUuid(const RunnerContext& context) {
   FsManagerOpts opts;
   opts.read_only = true;
   FsManager fs_manager(Env::Default(), opts);
@@ -73,15 +64,15 @@ Status PrintUuid(const vector<Mode*>& chain,
 unique_ptr<Mode> BuildFsMode() {
   unique_ptr<Action> format = ActionBuilder(
       { "format", "Format a new Kudu filesystem" }, &Format)
-    .AddGflag("fs_wal_dir")
-    .AddGflag("fs_data_dirs")
-    .AddGflag("uuid")
+    .AddOptionalParameter("fs_wal_dir")
+    .AddOptionalParameter("fs_data_dirs")
+    .AddOptionalParameter("uuid")
     .Build();
 
   unique_ptr<Action> print_uuid = ActionBuilder(
       { "print_uuid", "Print the UUID of a Kudu filesystem" }, &PrintUuid)
-    .AddGflag("fs_wal_dir")
-    .AddGflag("fs_data_dirs")
+    .AddOptionalParameter("fs_wal_dir")
+    .AddOptionalParameter("fs_data_dirs")
     .Build();
 
   return ModeBuilder({ "fs", "Operate on a local Kudu filesystem" })

http://git-wip-us.apache.org/repos/asf/kudu/blob/00dd9fde/src/kudu/tools/tool_action_tablet.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tools/tool_action_tablet.cc b/src/kudu/tools/tool_action_tablet.cc
index a34ced7..d019ff5 100644
--- a/src/kudu/tools/tool_action_tablet.cc
+++ b/src/kudu/tools/tool_action_tablet.cc
@@ -17,7 +17,6 @@
 
 #include "kudu/tools/tool_action.h"
 
-#include <deque>
 #include <iostream>
 #include <list>
 #include <memory>
@@ -27,7 +26,10 @@
 #include "kudu/common/wire_protocol.h"
 #include "kudu/consensus/consensus_meta.h"
 #include "kudu/fs/fs_manager.h"
+#include "kudu/gutil/map-util.h"
 #include "kudu/gutil/strings/join.h"
+#include "kudu/gutil/strings/split.h"
+#include "kudu/gutil/strings/stringpiece.h"
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/master/sys_catalog.h"
 #include "kudu/rpc/messenger.h"
@@ -44,13 +46,13 @@ using kudu::rpc::Messenger;
 using kudu::rpc::MessengerBuilder;
 using kudu::tserver::TabletCopyClient;
 using std::cout;
-using std::deque;
 using std::endl;
 using std::list;
 using std::shared_ptr;
 using std::string;
 using std::unique_ptr;
 using std::vector;
+using strings::Split;
 using strings::Substitute;
 
 namespace kudu {
@@ -85,7 +87,7 @@ Status ParseHostPortString(const string& hostport_str, HostPort* hostport)
{
 Status ParsePeerString(const string& peer_str,
                        string* uuid,
                        HostPort* hostport) {
-  int first_colon_idx = peer_str.find(":");
+  string::size_type first_colon_idx = peer_str.find(":");
   if (first_colon_idx == string::npos) {
     return Status::InvalidArgument(Substitute("bad peer '$0'", peer_str));
   }
@@ -95,13 +97,8 @@ Status ParsePeerString(const string& peer_str,
   return Status::OK();
 }
 
-Status PrintReplicaUuids(const vector<Mode*>& chain,
-                         const Action* action,
-                         deque<string> args) {
-  // Parse tablet ID argument.
-  string tablet_id;
-  RETURN_NOT_OK(ParseAndRemoveArg("tablet ID", &args, &tablet_id));
-  RETURN_NOT_OK(CheckNoMoreArgs(chain, action, args));
+Status PrintReplicaUuids(const RunnerContext& context) {
+  string tablet_id = FindOrDie(context.required_args, "tablet_id");
 
   FsManagerOpts opts;
   opts.read_only = true;
@@ -118,12 +115,9 @@ Status PrintReplicaUuids(const vector<Mode*>& chain,
   return Status::OK();
 }
 
-Status RewriteRaftConfig(const vector<Mode*>& chain,
-                         const Action* action,
-                         deque<string> args) {
+Status RewriteRaftConfig(const RunnerContext& context) {
   // Parse tablet ID argument.
-  string tablet_id;
-  RETURN_NOT_OK(ParseAndRemoveArg("tablet ID", &args, &tablet_id));
+  string tablet_id = FindOrDie(context.required_args, "tablet_id");
   if (tablet_id != master::SysCatalogTable::kSysCatalogTabletId) {
     LOG(WARNING) << "Master will not notice rewritten Raft config of regular "
                  << "tablets. A regular Raft config change must occur.";
@@ -131,16 +125,13 @@ Status RewriteRaftConfig(const vector<Mode*>& chain,
 
   // Parse peer arguments.
   vector<pair<string, HostPort>> peers;
-  for (const auto& arg : args) {
+  for (const auto& arg : context.variadic_args) {
     pair<string, HostPort> parsed_peer;
     RETURN_NOT_OK(ParsePeerString(arg,
                                   &parsed_peer.first, &parsed_peer.second));
     peers.push_back(parsed_peer);
   }
-  if (peers.empty()) {
-    return Status::InvalidArgument(Substitute(
-        "must provide at least one peer of form uuid:hostname:port"));
-  }
+  DCHECK(!peers.empty());
 
   // Make a copy of the old file before rewriting it.
   Env* env = Env::Default();
@@ -175,16 +166,10 @@ Status RewriteRaftConfig(const vector<Mode*>& chain,
   return cmeta->Flush();
 }
 
-Status Copy(const vector<Mode*>& chain,
-            const Action* action,
-            deque<string> args) {
+Status Copy(const RunnerContext& context) {
   // Parse the tablet ID and source arguments.
-  string tablet_id;
-  RETURN_NOT_OK(ParseAndRemoveArg("tablet ID", &args, &tablet_id));
-  string rpc_address;
-  RETURN_NOT_OK(ParseAndRemoveArg("source RPC address of form hostname:port",
-                                  &args, &rpc_address));
-  RETURN_NOT_OK(CheckNoMoreArgs(chain, action, args));
+  string tablet_id = FindOrDie(context.required_args, "tablet_id");
+  string rpc_address = FindOrDie(context.required_args, "source");
 
   HostPort hp;
   RETURN_NOT_OK(ParseHostPortString(rpc_address, &hp));
@@ -204,21 +189,23 @@ Status Copy(const vector<Mode*>& chain,
 } // anonymous namespace
 
 unique_ptr<Mode> BuildTabletMode() {
-  // TODO: Need to include required arguments in the help for these actions.
-
   unique_ptr<Action> print_replica_uuids = ActionBuilder(
       { "print_replica_uuids",
         "Print all replica UUIDs found in a tablet's Raft configuration" },
       &PrintReplicaUuids)
-    .AddGflag("fs_wal_dir")
-    .AddGflag("fs_data_dirs")
+    .AddRequiredParameter({ "tablet_id", "Tablet identifier" })
+    .AddOptionalParameter("fs_wal_dir")
+    .AddOptionalParameter("fs_data_dirs")
     .Build();
 
   unique_ptr<Action> rewrite_raft_config = ActionBuilder(
       { "rewrite_raft_config", "Rewrite a replica's Raft configuration" },
       &RewriteRaftConfig)
-    .AddGflag("fs_wal_dir")
-    .AddGflag("fs_data_dirs")
+    .AddRequiredParameter({ "tablet_id", "Tablet identifier" })
+    .AddRequiredVariadicParameter({
+        "peers", "List of peers where each peer is of form uuid:hostname:port" })
+    .AddOptionalParameter("fs_wal_dir")
+    .AddOptionalParameter("fs_data_dirs")
     .Build();
 
   unique_ptr<Mode> cmeta = ModeBuilder(
@@ -229,8 +216,10 @@ unique_ptr<Mode> BuildTabletMode() {
 
   unique_ptr<Action> copy = ActionBuilder(
       { "copy", "Copy a replica from a remote server" }, &Copy)
-    .AddGflag("fs_wal_dir")
-    .AddGflag("fs_data_dirs")
+    .AddRequiredParameter({ "tablet_id", "Tablet identifier" })
+    .AddRequiredParameter({ "source", "Source RPC address of form hostname:port" })
+    .AddOptionalParameter("fs_wal_dir")
+    .AddOptionalParameter("fs_data_dirs")
     .Build();
 
   return ModeBuilder({ "tablet", "Operate on a local Kudu replica" })

http://git-wip-us.apache.org/repos/asf/kudu/blob/00dd9fde/src/kudu/tools/tool_main.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tools/tool_main.cc b/src/kudu/tools/tool_main.cc
index 96a74fa..8743b7a 100644
--- a/src/kudu/tools/tool_main.cc
+++ b/src/kudu/tools/tool_main.cc
@@ -16,13 +16,17 @@
 // under the License.
 
 #include <deque>
-#include <gflags/gflags.h>
-#include <glog/logging.h>
 #include <iostream>
 #include <memory>
 #include <string>
+#include <unordered_map>
 #include <vector>
 
+#include <gflags/gflags.h>
+#include <glog/logging.h>
+
+#include "kudu/gutil/map-util.h"
+#include "kudu/gutil/strings/join.h"
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/tools/tool_action.h"
 #include "kudu/util/flags.h"
@@ -42,16 +46,60 @@ using std::deque;
 using std::endl;
 using std::string;
 using std::unique_ptr;
+using std::unordered_map;
 using std::vector;
 using strings::Substitute;
 
 namespace kudu {
 namespace tools {
 
+Status MarshalArgs(const vector<Mode*>& chain,
+                   Action* action,
+                   deque<string> input,
+                   unordered_map<string, string>* required,
+                   vector<string>* variadic) {
+  const ActionArgsDescriptor& args = action->args();
+
+  // Marshal the required arguments from the command line.
+  for (const auto& a : args.required) {
+    if (input.empty()) {
+      return Status::InvalidArgument(Substitute("must provide $0", a.name));
+    }
+    InsertOrDie(required, a.name, input.front());
+    input.pop_front();
+  }
+
+  // Marshal the variable length arguments, if they exist.
+  if (args.variadic) {
+    const ActionArgsDescriptor::Arg& a = args.variadic.get();
+    if (input.empty()) {
+      return Status::InvalidArgument(Substitute("must provide $0", a.name));
+    }
+
+    variadic->assign(input.begin(), input.end());
+    input.clear();
+  }
+
+  // There should be no unparsed arguments left.
+  if (!input.empty()) {
+    DCHECK(!chain.empty());
+    return Status::InvalidArgument(
+        Substitute("too many arguments: '$0'\n$1",
+                   JoinStrings(input, " "), action->BuildHelp(chain)));
+  }
+  return Status::OK();
+}
+
 int DispatchCommand(const vector<Mode*>& chain,
                     Action* action,
-                    const deque<string>& args) {
-  Status s = action->Run(chain, args);
+                    const deque<string>& remaining_args) {
+  unordered_map<string, string> required_args;
+  vector<string> variadic_args;
+  Status s = MarshalArgs(chain, action, remaining_args,
+                         &required_args, &variadic_args);
+  if (s.ok()) {
+    s = action->Run(chain, required_args, variadic_args);
+  }
   if (s.ok()) {
     return 0;
   } else {

http://git-wip-us.apache.org/repos/asf/kudu/blob/00dd9fde/src/kudu/util/subprocess.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/subprocess.cc b/src/kudu/util/subprocess.cc
index 014568a..a00b66c 100644
--- a/src/kudu/util/subprocess.cc
+++ b/src/kudu/util/subprocess.cc
@@ -20,6 +20,7 @@
 #include <dirent.h>
 #include <fcntl.h>
 #include <glog/logging.h>
+#include <glog/stl_logging.h>
 #include <memory>
 #include <signal.h>
 #include <string>
@@ -367,12 +368,12 @@ Status Subprocess::Kill(int signal) {
 }
 
 Status Subprocess::Call(const string& arg_str) {
-  VLOG(2) << "Invoking command: " << arg_str;
   vector<string> argv = Split(arg_str, " ");
   return Call(argv);
 }
 
 Status Subprocess::Call(const vector<string>& argv) {
+  VLOG(2) << "Invoking command: " << argv;
   Subprocess proc(argv[0], argv);
   RETURN_NOT_OK(proc.Start());
   int retcode;
@@ -389,6 +390,7 @@ Status Subprocess::Call(const vector<string>& argv) {
 }
 
 Status Subprocess::Call(const vector<string>& argv, string* stdout_out) {
+  VLOG(2) << "Invoking command: " << argv;
   Subprocess p(argv[0], argv);
   p.ShareParentStdout(false);
   RETURN_NOT_OK_PREPEND(p.Start(), "Unable to fork " + argv[0]);


Mime
View raw message