Details

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

      Description

      CommandFormat currently takes an array and offset for parsing and returns a list of arguments. It'd be much more convenient to have it process a list too. It would also be nice to differentiate between too few and too many args instead of the generic "Illegal number of arguments". Finally, CommandFormat is completely devoid of tests.

      1. HADOOP-7180-2.patch
        12 kB
        Daryn Sharp
      2. HADOOP-7180.patch
        11 kB
        Daryn Sharp

        Issue Links

          Activity

          Arun C Murthy made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #634 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/634/)
          Commit the missing file TestCommandFormat.java for HADOOP-7180.
          HADOOP-7180. Better support on CommandFormat on the API and exceptions. Contributed by Daryn Sharp

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #634 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/634/ ) Commit the missing file TestCommandFormat.java for HADOOP-7180 . HADOOP-7180 . Better support on CommandFormat on the API and exceptions. Contributed by Daryn Sharp
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #530 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/530/)
          Commit the missing file TestCommandFormat.java for HADOOP-7180.
          HADOOP-7180. Better support on CommandFormat on the API and exceptions. Contributed by Daryn Sharp

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #530 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/530/ ) Commit the missing file TestCommandFormat.java for HADOOP-7180 . HADOOP-7180 . Better support on CommandFormat on the API and exceptions. Contributed by Daryn Sharp
          Tsz Wo Nicholas Sze made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this. Thanks, Daryn!

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks, Daryn!
          Tsz Wo Nicholas Sze made changes -
          Hadoop Flags [Reviewed]
          Priority Major [ 3 ] Minor [ 4 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 3 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 contrib tests. The patch passed contrib unit tests.

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

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/311//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/311//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/311//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/12473957/HADOOP-7180-2.patch against trunk revision 1082329. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/311//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/311//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/311//console This message is automatically generated.
          Daryn Sharp made changes -
          Attachment HADOOP-7180-2.patch [ 12473957 ]
          Hide
          Daryn Sharp added a comment -

          Converted to junit 4, added what I think are the missing javadocs, added min/max & expected args to parameter exceptions.

          I omitted the suggested change to add an index to the list version of parse. The index is present on the old method to skip over ARGV[0] (the command). With the forthcoming changes, the command will be consumed from the list before calling the list-based parse. If it was added, then the sublist/erase would still be required since the list is expected to be destructively modified. If you feel strongly about the index, please let me know.

          Show
          Daryn Sharp added a comment - Converted to junit 4, added what I think are the missing javadocs , added min/max & expected args to parameter exceptions. I omitted the suggested change to add an index to the list version of parse. The index is present on the old method to skip over ARGV [0] (the command). With the forthcoming changes, the command will be consumed from the list before calling the list-based parse. If it was added, then the sublist/erase would still be required since the list is expected to be destructively modified. If you feel strongly about the index, please let me know.
          Tsz Wo Nicholas Sze made changes -
          Component/s fs [ 12310689 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -
          • Please use junit 4 (i.e. org.junit.Test and other classes org.junit.* instead of junit.framework.TestCase)
          • All public classes and methods (except tests) must have javadoc.
          • How about passing minPar/maxPar and psize to NotEnoughArgumentsException/TooManyArgumentsException and then shows the numbers in the error messages?
          • Minor: how about passing pos to parse(List<String> args), so that we could just return parse(Arrays.asList(args), pos) in parse(String[] args, int pos)?
          Show
          Tsz Wo Nicholas Sze added a comment - Please use junit 4 (i.e. org.junit.Test and other classes org.junit.* instead of junit.framework.TestCase ) All public classes and methods (except tests) must have javadoc. How about passing minPar / maxPar and psize to NotEnoughArgumentsException / TooManyArgumentsException and then shows the numbers in the error messages? Minor: how about passing pos to parse(List<String> args) , so that we could just return parse(Arrays.asList(args), pos) in parse(String[] args, int pos) ?
          Tsz Wo Nicholas Sze made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Assignee Daryn Sharp [ daryn ]
          Fix Version/s 0.23.0 [ 12315569 ]
          Tsz Wo Nicholas Sze made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Fix Version/s 0.22.0 [ 12314296 ]
          Daryn Sharp made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Daryn Sharp made changes -
          Attachment HADOOP-7180.patch [ 12473336 ]
          Hide
          Daryn Sharp added a comment -

          Add a slew of tests, add exceptions for too many/few, allowing parsing a list. Backwards compatibility.

          Show
          Daryn Sharp added a comment - Add a slew of tests, add exceptions for too many/few, allowing parsing a list. Backwards compatibility.
          Daryn Sharp made changes -
          Field Original Value New Value
          Link This issue is part of HADOOP-7176 [ HADOOP-7176 ]
          Daryn Sharp created issue -

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development