From 44a35b5d9accc4ecf7b1bbf762e593540bafe6a3 Mon Sep 17 00:00:00 2001 From: Todd Lipcon Date: Mon, 6 Jun 2011 20:53:37 +0000 Subject: [PATCH] HADOOP-7353. Cleanup FsShell and prevent masking of RTE stack traces. Contributed by Daryn Sharp. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1132764 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES.txt | 3 + src/java/org/apache/hadoop/fs/FsShell.java | 363 +++++++++--------- .../org/apache/hadoop/fs/shell/Command.java | 76 +++- .../hadoop/fs/shell/CommandFactory.java | 44 ++- .../org/apache/hadoop/fs/shell/Count.java | 5 +- .../org/apache/hadoop/fs/shell/Display.java | 2 +- .../org/apache/hadoop/fs/shell/FsCommand.java | 6 +- .../core/org/apache/hadoop/cli/testConf.xml | 4 +- 8 files changed, 282 insertions(+), 221 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 5e21b32de7..e2d5828c4a 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -280,6 +280,9 @@ Trunk (unreleased changes) HADOOP-7341. Fix options parsing in CommandFormat (Daryn Sharp via todd) + HADOOP-7353. Cleanup FsShell and prevent masking of RTE stack traces. + (Daryn Sharp via todd) + Release 0.22.0 - Unreleased INCOMPATIBLE CHANGES diff --git a/src/java/org/apache/hadoop/fs/FsShell.java b/src/java/org/apache/hadoop/fs/FsShell.java index b50f0b4a6d..376ea79586 100644 --- a/src/java/org/apache/hadoop/fs/FsShell.java +++ b/src/java/org/apache/hadoop/fs/FsShell.java @@ -18,9 +18,10 @@ package org.apache.hadoop.fs; import java.io.IOException; +import java.io.PrintStream; +import java.util.ArrayList; import java.util.Arrays; -import java.util.regex.Matcher; -import java.util.regex.Pattern; +import java.util.LinkedList; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -30,9 +31,6 @@ import org.apache.hadoop.fs.shell.Command; import org.apache.hadoop.fs.shell.CommandFactory; import org.apache.hadoop.fs.shell.FsCommand; -import org.apache.hadoop.fs.shell.PathExceptions.PathNotFoundException; -import org.apache.hadoop.ipc.RPC; -import org.apache.hadoop.util.StringUtils; import org.apache.hadoop.util.Tool; import org.apache.hadoop.util.ToolRunner; @@ -46,23 +44,31 @@ public class FsShell extends Configured implements Tool { private Trash trash; protected CommandFactory commandFactory; + private final String usagePrefix = + "Usage: hadoop fs [generic options]"; + /** + * Default ctor with no configuration. Be sure to invoke + * {@link #setConf(Configuration)} with a valid configuration prior + * to running commands. */ public FsShell() { this(null); } + /** + * Construct a FsShell with the given configuration. Commands can be + * executed via {@link #run(String[])} + * @param conf the hadoop configuration + */ public FsShell(Configuration conf) { super(conf); - fs = null; - trash = null; - commandFactory = new CommandFactory(); } protected FileSystem getFS() throws IOException { - if(fs == null) + if (fs == null) { fs = FileSystem.get(getConf()); - + } return fs; } @@ -75,93 +81,145 @@ protected Trash getTrash() throws IOException { protected void init() throws IOException { getConf().setQuietMode(true); + if (commandFactory == null) { + commandFactory = new CommandFactory(getConf()); + commandFactory.addObject(new Help(), "-help"); + commandFactory.addObject(new Usage(), "-usage"); + registerCommands(commandFactory); + } } + protected void registerCommands(CommandFactory factory) { + // TODO: DFSAdmin subclasses FsShell so need to protect the command + // registration. This class should morph into a base class for + // commands, and then this method can be abstract + if (this.getClass().equals(FsShell.class)) { + factory.registerCommands(FsCommand.class); + } + } + /** * Returns the Trash object associated with this shell. + * @return Path to the trash + * @throws IOException upon error */ public Path getCurrentTrashDir() throws IOException { return getTrash().getCurrentTrashDir(); } - /** - * Return an abbreviated English-language desc of the byte length - * @deprecated Consider using {@link org.apache.hadoop.util.StringUtils#byteDesc} instead. - */ - @Deprecated - public static String byteDesc(long len) { - return StringUtils.byteDesc(len); - } - - /** - * @deprecated Consider using {@link org.apache.hadoop.util.StringUtils#limitDecimalTo2} instead. - */ - @Deprecated - public static synchronized String limitDecimalTo2(double d) { - return StringUtils.limitDecimalTo2(d); - } - - private void printHelp(String cmd) { - String summary = "hadoop fs is the command to execute fs commands. " + - "The full syntax is: \n\n" + - "hadoop fs [-fs ] [-conf ]\n\t" + - "[-D ]\n\t" + - "[-report]"; - - String conf ="-conf : Specify an application configuration file."; - - String D = "-D : Use value for given property."; + // NOTE: Usage/Help are inner classes to allow access to outer methods + // that access commandFactory - String fs = "-fs [local | ]: \tSpecify the file system to use.\n" + - "\t\tIf not specified, the current configuration is used, \n" + - "\t\ttaken from the following, in increasing precedence: \n" + - "\t\t\tcore-default.xml inside the hadoop jar file \n" + - "\t\t\tcore-site.xml in $HADOOP_CONF_DIR \n" + - "\t\t'local' means use the local file system as your DFS. \n" + - "\t\t specifies a particular file system to \n" + - "\t\tcontact. This argument is optional but if used must appear\n" + - "\t\tappear first on the command line. Exactly one additional\n" + - "\t\targument must be specified. \n"; + /** + * Display help for commands with their short usage and long description + */ + protected class Usage extends FsCommand { + public static final String NAME = "usage"; + public static final String USAGE = "[cmd ...]"; + public static final String DESCRIPTION = + "Displays the usage for given command or all commands if none\n" + + "is specified."; + + @Override + protected void processRawArguments(LinkedList args) { + if (args.isEmpty()) { + printUsage(System.out); + } else { + for (String arg : args) printUsage(System.out, arg); + } + } + } - String help = "-help [cmd]: \tDisplays help for given command or all commands if none\n" + - "\t\tis specified.\n"; + /** + * Displays short usage of commands sans the long description + */ + protected class Help extends FsCommand { + public static final String NAME = "help"; + public static final String USAGE = "[cmd ...]"; + public static final String DESCRIPTION = + "Displays help for given command or all commands if none\n" + + "is specified."; + + @Override + protected void processRawArguments(LinkedList args) { + if (args.isEmpty()) { + printHelp(System.out); + } else { + for (String arg : args) printHelp(System.out, arg); + } + } + } - Command instance = commandFactory.getInstance("-" + cmd); - if (instance != null) { - printHelp(instance); - } else if ("fs".equals(cmd)) { - System.out.println(fs); - } else if ("conf".equals(cmd)) { - System.out.println(conf); - } else if ("D".equals(cmd)) { - System.out.println(D); - } else if ("help".equals(cmd)) { - System.out.println(help); + /* + * The following are helper methods for getInfo(). They are defined + * outside of the scope of the Help/Usage class because the run() method + * needs to invoke them too. + */ + + // print all usages + private void printUsage(PrintStream out) { + printInfo(out, null, false); + } + + // print one usage + private void printUsage(PrintStream out, String cmd) { + printInfo(out, cmd, false); + } + + // print all helps + private void printHelp(PrintStream out) { + printInfo(out, null, true); + } + + // print one help + private void printHelp(PrintStream out, String cmd) { + printInfo(out, cmd, true); + } + + private void printInfo(PrintStream out, String cmd, boolean showHelp) { + if (cmd != null) { + // display help or usage for one command + Command instance = commandFactory.getInstance("-" + cmd); + if (instance == null) { + throw new UnknownCommandException(cmd); + } + if (showHelp) { + printInstanceHelp(out, instance); + } else { + printInstanceUsage(out, instance); + } } else { - System.out.println(summary); - for (String thisCmdName : commandFactory.getNames()) { - instance = commandFactory.getInstance(thisCmdName); + // display help or usage for all commands + out.println(usagePrefix); + + // display list of short usages + ArrayList instances = new ArrayList(); + for (String name : commandFactory.getNames()) { + Command instance = commandFactory.getInstance(name); if (!instance.isDeprecated()) { System.out.println("\t[" + instance.getUsage() + "]"); + instances.add(instance); } } - System.out.println("\t[-help [cmd]]\n"); - - System.out.println(fs); + // display long descriptions for each command + if (showHelp) { + for (Command instance : instances) { + out.println(); + printInstanceHelp(out, instance); + } + } + out.println(); + ToolRunner.printGenericCommandUsage(out); + } + } - for (String thisCmdName : commandFactory.getNames()) { - instance = commandFactory.getInstance(thisCmdName); - if (!instance.isDeprecated()) { - printHelp(instance); - } - } - System.out.println(help); - } + private void printInstanceUsage(PrintStream out, Command instance) { + out.println(usagePrefix + " " + instance.getUsage()); } // TODO: will eventually auto-wrap the text, but this matches the expected // output for the hdfs tests... - private void printHelp(Command instance) { + private void printInstanceHelp(PrintStream out, Command instance) { boolean firstLine = true; for (String line : instance.getDescription().split("\n")) { String prefix; @@ -174,120 +232,51 @@ private void printHelp(Command instance) { System.out.println(prefix + line); } } - - /** - * Displays format of commands. - * - */ - private void printUsage(String cmd) { - String prefix = "Usage: java " + FsShell.class.getSimpleName(); - - Command instance = commandFactory.getInstance(cmd); - if (instance != null) { - System.err.println(prefix + " [" + instance.getUsage() + "]"); - } else if ("-fs".equals(cmd)) { - System.err.println("Usage: java FsShell" + - " [-fs ]"); - } else if ("-conf".equals(cmd)) { - System.err.println("Usage: java FsShell" + - " [-conf ]"); - } else if ("-D".equals(cmd)) { - System.err.println("Usage: java FsShell" + - " [-D <[property=value>]"); - } else { - System.err.println("Usage: java FsShell"); - for (String name : commandFactory.getNames()) { - instance = commandFactory.getInstance(name); - if (!instance.isDeprecated()) { - System.err.println(" [" + instance.getUsage() + "]"); - } - } - System.err.println(" [-help [cmd]]"); - System.err.println(); - ToolRunner.printGenericCommandUsage(System.err); - } - } /** * run */ public int run(String argv[]) throws Exception { - // TODO: This isn't the best place, but this class is being abused with - // subclasses which of course override this method. There really needs - // to be a better base class for all commands - commandFactory.setConf(getConf()); - commandFactory.registerCommands(FsCommand.class); - - if (argv.length < 1) { - printUsage(""); - return -1; - } + // initialize FsShell + init(); int exitCode = -1; - int i = 0; - String cmd = argv[i++]; - // initialize FsShell - try { - init(); - } catch (RPC.VersionMismatch v) { - LOG.debug("Version mismatch", v); - System.err.println("Version Mismatch between client and server" + - "... command aborted."); - return exitCode; - } catch (IOException e) { - LOG.debug("Error", e); - System.err.println("Bad connection to FS. Command aborted. Exception: " + - e.getLocalizedMessage()); - return exitCode; - } - - try { - Command instance = commandFactory.getInstance(cmd); - if (instance != null) { - exitCode = instance.run(Arrays.copyOfRange(argv, i, argv.length)); - } else if ("-help".equals(cmd)) { - if (i < argv.length) { - printHelp(argv[i]); - } else { - printHelp(""); + if (argv.length < 1) { + printUsage(System.err); + } else { + String cmd = argv[0]; + Command instance = null; + try { + instance = commandFactory.getInstance(cmd); + if (instance == null) { + throw new UnknownCommandException(); } - } else { - System.err.println(cmd + ": Unknown command"); - printUsage(""); - } - } catch (Exception e) { - exitCode = 1; - LOG.debug("Error", e); - displayError(cmd, e); - if (e instanceof IllegalArgumentException) { - exitCode = -1; - printUsage(cmd); + exitCode = instance.run(Arrays.copyOfRange(argv, 1, argv.length)); + } catch (IllegalArgumentException e) { + displayError(cmd, e.getLocalizedMessage()); + if (instance != null) { + printInstanceUsage(System.err, instance); + } + } catch (Exception e) { + // instance.run catches IOE, so something is REALLY wrong if here + LOG.debug("Error", e); + displayError(cmd, "Fatal internal error"); + e.printStackTrace(System.err); } } return exitCode; } - - // TODO: this is a quick workaround to accelerate the integration of - // redesigned commands. this will be removed this once all commands are - // converted. this change will avoid having to change the hdfs tests - // every time a command is converted to use path-based exceptions - private static Pattern[] fnfPatterns = { - Pattern.compile("File (.*) does not exist\\."), - Pattern.compile("File does not exist: (.*)"), - Pattern.compile("`(.*)': specified destination directory doest not exist") - }; - private void displayError(String cmd, Exception e) { - String message = e.getLocalizedMessage().split("\n")[0]; - for (Pattern pattern : fnfPatterns) { - Matcher matcher = pattern.matcher(message); - if (matcher.matches()) { - message = new PathNotFoundException(matcher.group(1)).getMessage(); - break; - } + + private void displayError(String cmd, String message) { + for (String line : message.split("\n")) { + System.err.println(cmd.substring(1) + ": " + line); } - System.err.println(cmd.substring(1) + ": " + message); } + /** + * Performs any necessary cleanup + * @throws IOException upon error + */ public void close() throws IOException { if (fs != null) { fs.close(); @@ -297,9 +286,11 @@ public void close() throws IOException { /** * main() has some simple utility methods + * @param argv the command and its arguments + * @throws Exception upon error */ public static void main(String argv[]) throws Exception { - FsShell shell = new FsShell(); + FsShell shell = newShellInstance(); int res; try { res = ToolRunner.run(shell, argv); @@ -308,4 +299,26 @@ public static void main(String argv[]) throws Exception { } System.exit(res); } + + // TODO: this should be abstract in a base class + protected static FsShell newShellInstance() { + return new FsShell(); + } + + /** + * The default ctor signals that the command being executed does not exist, + * while other ctor signals that a specific command does not exist. The + * latter is used by commands that process other commands, ex. -usage/-help + */ + @SuppressWarnings("serial") + static class UnknownCommandException extends IllegalArgumentException { + private final String cmd; + UnknownCommandException() { this(null); } + UnknownCommandException(String cmd) { this.cmd = cmd; } + + @Override + public String getMessage() { + return ((cmd != null) ? "`"+cmd+"': " : "") + "Unknown command"; + } + } } diff --git a/src/java/org/apache/hadoop/fs/shell/Command.java b/src/java/org/apache/hadoop/fs/shell/Command.java index f2507b92c2..f9efdfcfb0 100644 --- a/src/java/org/apache/hadoop/fs/shell/Command.java +++ b/src/java/org/apache/hadoop/fs/shell/Command.java @@ -42,6 +42,13 @@ @InterfaceStability.Evolving abstract public class Command extends Configured { + /** default name of the command */ + public static String NAME; + /** the command's usage switches and arguments format */ + public static String USAGE; + /** the command's long description */ + public static String DESCRIPTION; + protected String[] args; protected String name; protected int exitCode = 0; @@ -70,14 +77,6 @@ protected Command(Configuration conf) { /** @return the command's name excluding the leading character - */ abstract public String getCommandName(); - /** - * Name the command - * @param cmdName as invoked - */ - public void setCommandName(String cmdName) { - name = cmdName; - } - protected void setRecursive(boolean flag) { recursive = flag; } @@ -120,14 +119,16 @@ public int runAll() { * expand arguments, and then process each argument. * 
    * run
-   * \-> {@link #processOptions(LinkedList)}
-   * \-> {@link #expandArguments(LinkedList)} -> {@link #expandArgument(String)}*
-   * \-> {@link #processArguments(LinkedList)}
-   *     \-> {@link #processArgument(PathData)}*
-   *         \-> {@link #processPathArgument(PathData)}
-   *             \-> {@link #processPaths(PathData, PathData...)}
-   *                 \-> {@link #processPath(PathData)}*
-   *         \-> {@link #processNonexistentPath(PathData)}
+   * |-> {@link #processOptions(LinkedList)}
+   * \-> {@link #processRawArguments(LinkedList)}
+   *      |-> {@link #expandArguments(LinkedList)}
+   *      |   \-> {@link #expandArgument(String)}*
+   *      \-> {@link #processArguments(LinkedList)}
+   *          |-> {@link #processArgument(PathData)}*
+   *          |   |-> {@link #processPathArgument(PathData)}
+   *          |   \-> {@link #processPaths(PathData, PathData...)}
+   *          |        \-> {@link #processPath(PathData)}*
+   *          \-> {@link #processNonexistentPath(PathData)}
    *
* Most commands will chose to implement just * {@link #processOptions(LinkedList)} and {@link #processPath(PathData)} @@ -144,7 +145,7 @@ public int run(String...argv) { "DEPRECATED: Please use '"+ getReplacementCommand() + "' instead."); } processOptions(args); - processArguments(expandArguments(args)); + processRawArguments(args); } catch (IOException e) { displayError(e); } @@ -170,6 +171,19 @@ public int run(String...argv) { */ protected void processOptions(LinkedList args) throws IOException {} + /** + * Allows commands that don't use paths to handle the raw arguments. + * Default behavior is to expand the arguments via + * {@link #expandArguments(LinkedList)} and pass the resulting list to + * {@link #processArguments(LinkedList)} + * @param args the list of argument strings + * @throws IOException + */ + protected void processRawArguments(LinkedList args) + throws IOException { + processArguments(expandArguments(args)); + } + /** * Expands a list of arguments into {@link PathData} objects. The default * behavior is to call {@link #expandArgument(String)} on each element @@ -353,7 +367,26 @@ public void displayError(String message) { * @param message warning message to display */ public void displayWarning(String message) { - err.println(getCommandName() + ": " + message); + err.println(getName() + ": " + message); + } + + /** + * The name of the command. Will first try to use the assigned name + * else fallback to the command's preferred name + * @return name of the command + */ + public String getName() { + return (name == null) + ? getCommandField("NAME") + : name.startsWith("-") ? name.substring(1) : name; // this is a historical method + } + + /** + * Define the name of the command. + * @param name as invoked + */ + public void setName(String name) { + this.name = name; } /** @@ -361,7 +394,7 @@ public void displayWarning(String message) { * @return "name options" */ public String getUsage() { - String cmd = "-" + getCommandName(); + String cmd = "-" + getName(); String usage = isDeprecated() ? "" : getCommandField("USAGE"); return usage.isEmpty() ? cmd : cmd + " " + usage; } @@ -400,9 +433,10 @@ public String getReplacementCommand() { private String getCommandField(String field) { String value; try { - value = (String)this.getClass().getField(field).get(null); + value = this.getClass().getField(field).get(this).toString(); } catch (Exception e) { - throw new RuntimeException(StringUtils.stringifyException(e)); + throw new RuntimeException( + "failed to get " + this.getClass().getSimpleName()+"."+field, e); } return value; } diff --git a/src/java/org/apache/hadoop/fs/shell/CommandFactory.java b/src/java/org/apache/hadoop/fs/shell/CommandFactory.java index c8425145cc..f5d9d5a801 100644 --- a/src/java/org/apache/hadoop/fs/shell/CommandFactory.java +++ b/src/java/org/apache/hadoop/fs/shell/CommandFactory.java @@ -19,7 +19,8 @@ package org.apache.hadoop.fs.shell; import java.util.Arrays; -import java.util.Hashtable; +import java.util.HashMap; +import java.util.Map; import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; @@ -35,8 +36,11 @@ @InterfaceStability.Unstable public class CommandFactory extends Configured implements Configurable { - private Hashtable> classMap = - new Hashtable>(); + private Map> classMap = + new HashMap>(); + + private Map objectMap = + new HashMap(); /** Factory constructor for commands */ public CommandFactory() { @@ -79,16 +83,22 @@ public void addClass(Class cmdClass, String ... names) { } /** - * Returns the class implementing the given command. The - * class must have been registered via - * {@link #addClass(Class, String...)} - * @param cmd name of the command - * @return instance of the requested command + * Register the given object as handling the given list of command + * names. Avoid calling this method and use + * {@link #addClass(Class, String...)} whenever possible to avoid + * startup overhead from excessive command object instantiations. This + * method is intended only for handling nested non-static classes that + * are re-usable. Namely -help/-usage. + * @param cmdObject the object implementing the command names + * @param names one or more command names that will invoke this class */ - protected Class getClass(String cmd) { - return classMap.get(cmd); + public void addObject(Command cmdObject, String ... names) { + for (String name : names) { + objectMap.put(name, cmdObject); + classMap.put(name, null); // just so it shows up in the list of commands + } } - + /** * Returns an instance of the class implementing the given command. The * class must have been registered via @@ -109,11 +119,13 @@ public Command getInstance(String cmd) { public Command getInstance(String cmdName, Configuration conf) { if (conf == null) throw new NullPointerException("configuration is null"); - Command instance = null; - Class cmdClass = getClass(cmdName); - if (cmdClass != null) { - instance = ReflectionUtils.newInstance(cmdClass, conf); - instance.setCommandName(cmdName); + Command instance = objectMap.get(cmdName); + if (instance == null) { + Class cmdClass = classMap.get(cmdName); + if (cmdClass != null) { + instance = ReflectionUtils.newInstance(cmdClass, conf); + instance.setName(cmdName); + } } return instance; } diff --git a/src/java/org/apache/hadoop/fs/shell/Count.java b/src/java/org/apache/hadoop/fs/shell/Count.java index 891e68a4bf..219973f25d 100644 --- a/src/java/org/apache/hadoop/fs/shell/Count.java +++ b/src/java/org/apache/hadoop/fs/shell/Count.java @@ -54,9 +54,7 @@ public static void registerCommands(CommandFactory factory) { private boolean showQuotas; /** Constructor */ - public Count() { - setCommandName(NAME); - } + public Count() {} /** Constructor * @deprecated invoke via {@link FsShell} @@ -67,7 +65,6 @@ public Count() { @Deprecated public Count(String[] cmd, int pos, Configuration conf) { super(conf); - setCommandName(NAME); this.args = Arrays.copyOfRange(cmd, pos, cmd.length); } diff --git a/src/java/org/apache/hadoop/fs/shell/Display.java b/src/java/org/apache/hadoop/fs/shell/Display.java index e650b711bc..8a05a55310 100644 --- a/src/java/org/apache/hadoop/fs/shell/Display.java +++ b/src/java/org/apache/hadoop/fs/shell/Display.java @@ -100,7 +100,7 @@ protected InputStream getInputStream(PathData item) throws IOException { */ public static class Text extends Cat { public static final String NAME = "text"; - public static final String SHORT_USAGE = Cat.USAGE; + public static final String USAGE = Cat.USAGE; public static final String DESCRIPTION = "Takes a source file and outputs the file in text format.\n" + "The allowed formats are zip and TextRecordInputStream."; diff --git a/src/java/org/apache/hadoop/fs/shell/FsCommand.java b/src/java/org/apache/hadoop/fs/shell/FsCommand.java index 32bec37182..3f397327de 100644 --- a/src/java/org/apache/hadoop/fs/shell/FsCommand.java +++ b/src/java/org/apache/hadoop/fs/shell/FsCommand.java @@ -65,8 +65,10 @@ protected FsCommand(Configuration conf) { super(conf); } - public String getCommandName() { - return name.startsWith("-") ? name.substring(1) : name; + // historical abstract method in Command + @Override + public String getCommandName() { + return getName(); } // abstract method that normally is invoked by runall() which is diff --git a/src/test/core/org/apache/hadoop/cli/testConf.xml b/src/test/core/org/apache/hadoop/cli/testConf.xml index 7567719bde..d8b5785682 100644 --- a/src/test/core/org/apache/hadoop/cli/testConf.xml +++ b/src/test/core/org/apache/hadoop/cli/testConf.xml @@ -39,7 +39,7 @@ SubstringComparator - hadoop fs is the command to execute fs commands. The full syntax is + Usage: hadoop fs [generic options] @@ -730,7 +730,7 @@ RegexpComparator - ^-help \[cmd\]:( |\t)*Displays help for given command or all commands if none( )* + ^-help \[cmd ...\]:( |\t)*Displays help for given command or all commands if none( )* RegexpComparator