Hadoop Common
  1. Hadoop Common
  2. HADOOP-7353

Cleanup FsShell and prevent masking of RTE stacktraces

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 0.23.0
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      FsShell's top level exception handler catches and displays exceptions. Unfortunately it displays only the first line of an exception, which means an unexpected RuntimeExceptions like NullPointerException only display "cmd: NullPointerException". This user has no context to understand and/or accurately report the issue.

      Found due to bugs such as HADOOP-7327.

      1. HADOOP-7353.patch
        26 kB
        Daryn Sharp
      2. HADOOP-7353-2.patch
        26 kB
        Daryn Sharp

        Issue Links

          Activity

          Hide
          Daryn Sharp added a comment -

          Cleanup exception handling in FsShell.run() method to print the stack trace of unexpected exceptions such as NPEs.

          Converted -help & -usage into FsCommand subclasses to greatly simplify the aforementioned exception handling block. All conditional chains are now gone! Everything is dynamic.

          A few trivial changes to Command & CommandFactory to support nested classes needed for -help/-usage to access the CommandFactory.

          Break shell object instantiation, and population of commandFactory, into separate methods so DFSAdmin (already a subclass) can eventually take advantage of the modularity.

          Show
          Daryn Sharp added a comment - Cleanup exception handling in FsShell.run() method to print the stack trace of unexpected exceptions such as NPEs. Converted -help & -usage into FsCommand subclasses to greatly simplify the aforementioned exception handling block. All conditional chains are now gone! Everything is dynamic. A few trivial changes to Command & CommandFactory to support nested classes needed for -help/-usage to access the CommandFactory . Break shell object instantiation, and population of commandFactory, into separate methods so DFSAdmin (already a subclass) can eventually take advantage of the modularity.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12481372/HADOOP-7353.patch
          against trunk revision 1130833.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 4 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/569//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/569//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/569//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12481372/HADOOP-7353.patch against trunk revision 1130833. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/569//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/569//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/569//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          I also removed the slew of pre-existing warnings in FsShell...

          Show
          Daryn Sharp added a comment - I also removed the slew of pre-existing warnings in FsShell ...
          Hide
          Aaron T. Myers added a comment -

          Looks pretty good, Daryn. A few small nits:

          • There's no need to expressly initialize the instance variables in the FsShell} ctor to {{null. Java will do that automatically.
          • "The follow methods are syntactic sugar." - They're not really "syntactic" sugar, as much as they are just helpers.
          • "if (instance == null) throw new UnknownCommandException(cmd);" - coding guidelines say to put always use braces on if statements, and always put the body on a new line.
          • There seems to be an inconsistency with printing to stdout vs. stderr, or at least I can't tell why you chose to use one in some places, and the other in other places.
          • "// historical abstract method in Command " - move this comment above the method.
          Show
          Aaron T. Myers added a comment - Looks pretty good, Daryn. A few small nits: There's no need to expressly initialize the instance variables in the FsShell} ctor to {{null . Java will do that automatically. "The follow methods are syntactic sugar." - They're not really " syntactic " sugar, as much as they are just helpers. "if (instance == null) throw new UnknownCommandException(cmd);" - coding guidelines say to put always use braces on if statements, and always put the body on a new line. There seems to be an inconsistency with printing to stdout vs. stderr, or at least I can't tell why you chose to use one in some places, and the other in other places. "// historical abstract method in Command " - move this comment above the method.
          Hide
          Daryn Sharp added a comment -

          Thanks Aaron!

          There's no need to expressly initialize the instance variables in the FsShell ctor to null. Java will do that automatically.

          Sure, I removed the initialization.

          "The follow methods are syntactic sugar." - They're not really "syntactic" sugar, as much as they are just helpers.

          Changed.

          "if (instance == null) throw new UnknownCommandException(cmd);" - coding guidelines say to put always use braces on if statements, and always put the body on a new line.

          Changed. I found a few more, so I changed them too.

          There seems to be an inconsistency with printing to stdout vs. stderr, or at least I can't tell why you chose to use one in some places, and the other in other places.

          The -help/-usage commands will print to stdout. The usage goes to stderr when no command is given, or because of an illegal argument.

          "// historical abstract method in Command " - move this comment above the method.

          Moved.

          Thanks again!

          Show
          Daryn Sharp added a comment - Thanks Aaron! There's no need to expressly initialize the instance variables in the FsShell ctor to null . Java will do that automatically. Sure, I removed the initialization. "The follow methods are syntactic sugar." - They're not really "syntactic" sugar, as much as they are just helpers. Changed. "if (instance == null) throw new UnknownCommandException(cmd);" - coding guidelines say to put always use braces on if statements, and always put the body on a new line. Changed. I found a few more, so I changed them too. There seems to be an inconsistency with printing to stdout vs. stderr, or at least I can't tell why you chose to use one in some places, and the other in other places. The -help/-usage commands will print to stdout. The usage goes to stderr when no command is given, or because of an illegal argument. "// historical abstract method in Command " - move this comment above the method. Moved. Thanks again!
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12481578/HADOOP-7353-2.patch
          against trunk revision 1132511.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 4 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/581//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/581//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/581//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12481578/HADOOP-7353-2.patch against trunk revision 1132511. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/581//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/581//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/581//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          +1, latest patch looks good to me.

          Show
          Aaron T. Myers added a comment - +1, latest patch looks good to me.
          Hide
          Todd Lipcon added a comment -

          Committed to trunk based on atm's +1 and a quick sanity check. Thanks Daryn/Aaron.

          Show
          Todd Lipcon added a comment - Committed to trunk based on atm's +1 and a quick sanity check. Thanks Daryn/Aaron.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #643 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/643/)
          HADOOP-7353. Cleanup FsShell and prevent masking of RTE stack traces. Contributed by Daryn Sharp.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1132764
          Files :

          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java
          • /hadoop/common/trunk/CHANGES.txt
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CommandFactory.java
          • /hadoop/common/trunk/src/test/core/org/apache/hadoop/cli/testConf.xml
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Command.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Display.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FsShell.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #643 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/643/ ) HADOOP-7353 . Cleanup FsShell and prevent masking of RTE stack traces. Contributed by Daryn Sharp. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1132764 Files : /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java /hadoop/common/trunk/CHANGES.txt /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CommandFactory.java /hadoop/common/trunk/src/test/core/org/apache/hadoop/cli/testConf.xml /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Command.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Display.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FsShell.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #712 (See https://builds.apache.org/job/Hadoop-Common-trunk/712/)
          HADOOP-7353. Cleanup FsShell and prevent masking of RTE stack traces. Contributed by Daryn Sharp.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1132764
          Files :

          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Count.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java
          • /hadoop/common/trunk/CHANGES.txt
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CommandFactory.java
          • /hadoop/common/trunk/src/test/core/org/apache/hadoop/cli/testConf.xml
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Command.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Display.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FsShell.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #712 (See https://builds.apache.org/job/Hadoop-Common-trunk/712/ ) HADOOP-7353 . Cleanup FsShell and prevent masking of RTE stack traces. Contributed by Daryn Sharp. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1132764 Files : /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Count.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java /hadoop/common/trunk/CHANGES.txt /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/CommandFactory.java /hadoop/common/trunk/src/test/core/org/apache/hadoop/cli/testConf.xml /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Command.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Display.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FsShell.java

            People

            • Assignee:
              Daryn Sharp
              Reporter:
              Daryn Sharp
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development