Details

    • Type: Improvement Improvement
    • 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:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      The "Found X items" header on the output of the "du" command has been removed to more closely match unix. The displayed paths now correspond to the command line arguments instead of always being a fully qualified URI. For example, the output will have relative paths if the command line arguments are relative paths.
      Show
      The "Found X items" header on the output of the "du" command has been removed to more closely match unix. The displayed paths now correspond to the command line arguments instead of always being a fully qualified URI. For example, the output will have relative paths if the command line arguments are relative paths.

      Description

      Need to refactor to conform to FsCommand subclass.

      1. HADOOP-7286.patch
        21 kB
        Daryn Sharp
      2. HADOOP-7286-2.patch
        22 kB
        Daryn Sharp
      3. HADOOP-7286-3.patch
        31 kB
        Daryn Sharp
      4. HADOOP-7286-4.patch
        31 kB
        Daryn Sharp

        Issue Links

          Activity

          Daryn Sharp created issue -
          Daryn Sharp made changes -
          Field Original Value New Value
          Link This issue incorporates HDFS-1931 [ HDFS-1931 ]
          Hide
          Daryn Sharp added a comment -

          refactor and make output a bit more conforming to unix standards

          Show
          Daryn Sharp added a comment - refactor and make output a bit more conforming to unix standards
          Daryn Sharp made changes -
          Attachment HADOOP-7286.patch [ 12479039 ]
          Daryn Sharp made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12479039/HADOOP-7286.patch
          against trunk revision 1102123.

          +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 appears to introduce 3 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://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/442//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/442//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/442//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/12479039/HADOOP-7286.patch against trunk revision 1102123. +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 appears to introduce 3 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://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/442//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/442//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/442//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          Fix findbug warning by changing String concat to StringBuilder. Also removed unrelated and unused method that is generating another findbug warning.

          Show
          Daryn Sharp added a comment - Fix findbug warning by changing String concat to StringBuilder. Also removed unrelated and unused method that is generating another findbug warning.
          Daryn Sharp made changes -
          Attachment HADOOP-7286-2.patch [ 12479137 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12479137/HADOOP-7286-2.patch
          against trunk revision 1102123.

          +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 failed these core unit tests:
          org.apache.hadoop.metrics2.impl.TestSinkQueue

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/447//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/447//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/447//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/12479137/HADOOP-7286-2.patch against trunk revision 1102123. +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 failed these core unit tests: org.apache.hadoop.metrics2.impl.TestSinkQueue +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/447//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/447//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/447//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          Metrics test failure is unrelated to this patch.

          Show
          Daryn Sharp added a comment - Metrics test failure is unrelated to this patch.
          Hide
          Aaron T. Myers added a comment -

          Patch looks pretty solid, Daryn. A few comments:

          1. I think the name "Capacity" isn't the best. At least in the case of, "du" and "dus", there's no capacity information being calculated - just usage info. I suggest renaming to something like "FsUsage". Thoughts?
          2. Since all sub-classes of o.a.h.fs.shell.Capacity need a TableBuilder object, might as well make this a protected instance variable of Capacity a la humanReadable.
          3. I've always found it silly that we continue to maintain both "du" and "dus". Perhaps now would be a good time to deprecate "dus" in favor of "du -s" ? Feel free to say this should be done as part of a separate JIRA.
          Show
          Aaron T. Myers added a comment - Patch looks pretty solid, Daryn. A few comments: I think the name " Capacity " isn't the best. At least in the case of, " du " and " dus ", there's no capacity information being calculated - just usage info. I suggest renaming to something like " FsUsage ". Thoughts? Since all sub-classes of o.a.h.fs.shell.Capacity need a TableBuilder object, might as well make this a protected instance variable of Capacity a la humanReadable . I've always found it silly that we continue to maintain both " du " and " dus ". Perhaps now would be a good time to deprecate " dus " in favor of " du -s " ? Feel free to say this should be done as part of a separate JIRA.
          Hide
          Daryn Sharp added a comment -

          All good ideas! I too dislike all the commands like du/dus that are just permutations of each other, and would love to get rid of them. How do you propose "dus" is deprecated? Writing something to stderr? Or just get rid of it? I'd favor the latter unless there are objections.

          Show
          Daryn Sharp added a comment - All good ideas! I too dislike all the commands like du/dus that are just permutations of each other, and would love to get rid of them. How do you propose "dus" is deprecated? Writing something to stderr? Or just get rid of it? I'd favor the latter unless there are objections.
          Hide
          Todd Lipcon added a comment -

          I think we need to leave "dus" for at least one stable version. IMO it should print something like:

          WARNING: The "dus" command has been deprecated. Please use "du s" instead. <- on stderr
          12345 file:/foo <-- on stdout

          Show
          Todd Lipcon added a comment - I think we need to leave "dus" for at least one stable version. IMO it should print something like: WARNING: The "dus" command has been deprecated. Please use "du s" instead. < - on stderr 12345 file:/foo <-- on stdout
          Hide
          Daryn Sharp added a comment -

          Addresses Aaron and Todd's comments. Added deprecation warnings for dus, rmr, lsr.

          Show
          Daryn Sharp added a comment - Addresses Aaron and Todd's comments. Added deprecation warnings for dus, rmr, lsr.
          Daryn Sharp made changes -
          Attachment HADOOP-7286-3.patch [ 12479380 ]
          Hide
          Aaron T. Myers added a comment -

          Latest patch looks to be almost there. A few nits:

          1. Comment in FsUsage.java still references "viewing capacity." You also might as well explicitly say something like "this is the base class for the df and du commands.
          2. I think the deprecatedFor(...) interface could be clearer. I think "boolean isDeprecated(...)" and something like "String getPreferredCommand(...)" would be a lot quicker to understand.
          Show
          Aaron T. Myers added a comment - Latest patch looks to be almost there. A few nits: Comment in FsUsage.java still references "viewing capacity." You also might as well explicitly say something like "this is the base class for the df and du commands. I think the deprecatedFor(...) interface could be clearer. I think " boolean isDeprecated(...) " and something like " String getPreferredCommand(...) " would be a lot quicker to understand.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12479380/HADOOP-7286-3.patch
          against trunk revision 1103856.

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

          +1 tests included. The patch appears to include 6 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/456//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/456//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/456//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/12479380/HADOOP-7286-3.patch against trunk revision 1103856. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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/456//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/456//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/456//console This message is automatically generated.
          Hide
          Daryn Sharp added a comment -

          Per Aaron, switched back to is/get methods for deprecation queries, and updated comments to be more succinct.

          Show
          Daryn Sharp added a comment - Per Aaron, switched back to is/get methods for deprecation queries, and updated comments to be more succinct.
          Daryn Sharp made changes -
          Attachment HADOOP-7286-4.patch [ 12479398 ]
          Hide
          Aaron T. Myers added a comment -

          +1, the patch looks good to me. Thanks a lot, Daryn.

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

          The patch changes the output in a couple ways that are worth considering:

          • It used to print a line saying "Found n items" at the top of the output
          • The paths used to be fully qualified but no longer are

          example output before patch for "hadoop fs -du /home/todd/tokyo-nosql/":

          Found 3 items
          3429427  file:/home/todd/tokyo-nosql/tokyo-nosql.pdf
          1255650  file:/home/todd/tokyo-nosql/tokyo-nosql-slides-only.pdf
          4798464  file:/home/todd/tokyo-nosql/tokyo-nosql.ppt
          

          After:

          3429427  /home/todd/tokyo-nosql/tokyo-nosql.pdf
          1255650  /home/todd/tokyo-nosql/tokyo-nosql-slides-only.pdf
          4798464  /home/todd/tokyo-nosql/tokyo-nosql.ppt
          

          Are these conscious changes that we're OK with?

          Show
          Todd Lipcon added a comment - The patch changes the output in a couple ways that are worth considering: It used to print a line saying "Found n items" at the top of the output The paths used to be fully qualified but no longer are example output before patch for "hadoop fs -du /home/todd/tokyo-nosql/": Found 3 items 3429427 file:/home/todd/tokyo-nosql/tokyo-nosql.pdf 1255650 file:/home/todd/tokyo-nosql/tokyo-nosql-slides-only.pdf 4798464 file:/home/todd/tokyo-nosql/tokyo-nosql.ppt After: 3429427 /home/todd/tokyo-nosql/tokyo-nosql.pdf 1255650 /home/todd/tokyo-nosql/tokyo-nosql-slides-only.pdf 4798464 /home/todd/tokyo-nosql/tokyo-nosql.ppt Are these conscious changes that we're OK with?
          Hide
          Daryn Sharp added a comment -

          The changes were deliberate on my part. Here's my thoughts: FsShell tries to implement posix/unix-like commands, but does so in its own "uniquely" inconsistent fashion which is a surprise to any shell coder. IMHO, if FsShell wants to be unix-like, then it's output should look unix-like, or at a minimum be self-consistent.

          I think du is the only command that is "noticably" different, particularly with regard to the subtraction of the header. I checked "du" on various systems and didn't find any that printed "Found XXX items". Going forward, I think du should be able to traverse more than one level deep, at which point it becomes prohibitively expensive for du to sponge memory to batch up all the output just so it can print a total on the first line.

          Regarding the path display, it's been a universal change to standardize how paths are displayed since the commands' errors and output used to be wildly inconsistent. Typically what you give on the cmdline is now what you get back, just like you would expect from a unix-like system. Although, I can tweak the du output to be absolute uris if necessary.

          Show
          Daryn Sharp added a comment - The changes were deliberate on my part. Here's my thoughts: FsShell tries to implement posix/unix-like commands, but does so in its own "uniquely" inconsistent fashion which is a surprise to any shell coder. IMHO, if FsShell wants to be unix-like, then it's output should look unix-like, or at a minimum be self-consistent. I think du is the only command that is "noticably" different, particularly with regard to the subtraction of the header. I checked "du" on various systems and didn't find any that printed "Found XXX items". Going forward, I think du should be able to traverse more than one level deep, at which point it becomes prohibitively expensive for du to sponge memory to batch up all the output just so it can print a total on the first line. Regarding the path display, it's been a universal change to standardize how paths are displayed since the commands' errors and output used to be wildly inconsistent. Typically what you give on the cmdline is now what you get back, just like you would expect from a unix-like system. Although, I can tweak the du output to be absolute uris if necessary.
          Hide
          Todd Lipcon added a comment -

          That seems OK to me. I've marked this JIRA as an incompatible change - please add a release note that details the change in format.

          Show
          Todd Lipcon added a comment - That seems OK to me. I've marked this JIRA as an incompatible change - please add a release note that details the change in format.
          Todd Lipcon made changes -
          Hadoop Flags [Incompatible change]
          Daryn Sharp made changes -
          Release Note The "Found X items" header has been removed to more closely match unix. The displayed paths now correspond to the command line arguments instead of always being a fully qualified URI. Ie. The output will have relative paths if the command line arguments are relative paths.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12479398/HADOOP-7286-4.patch
          against trunk revision 1103931.

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

          +1 tests included. The patch appears to include 6 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/459//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/459//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/459//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/12479398/HADOOP-7286-4.patch against trunk revision 1103931. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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/459//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/459//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/459//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          +1, committing to trunk. Thanks, Daryn!

          Show
          Todd Lipcon added a comment - +1, committing to trunk. Thanks, Daryn!
          Todd Lipcon made changes -
          Hadoop Flags [Incompatible change] [Incompatible change, Reviewed]
          Release Note The "Found X items" header has been removed to more closely match unix. The displayed paths now correspond to the command line arguments instead of always being a fully qualified URI. Ie. The output will have relative paths if the command line arguments are relative paths. The "Found X items" header on the output of the "du" command has been removed to more closely match unix. The displayed paths now correspond to the command line arguments instead of always being a fully qualified URI. For example, the output will have relative paths if the command line arguments are relative paths.
          Todd Lipcon made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s 0.23.0 [ 12315569 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #603 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/603/)
          HADOOP-7286. Refactor the du/dus/df commands to conform to new FsCommand class. Contributed by Daryn Sharp.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #603 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/603/ ) HADOOP-7286 . Refactor the du/dus/df commands to conform to new FsCommand class. Contributed by Daryn Sharp.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #691 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/691/)
          HADOOP-7286. Refactor the du/dus/df commands to conform to new FsCommand class. Contributed by Daryn Sharp.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #691 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/691/ ) HADOOP-7286 . Refactor the du/dus/df commands to conform to new FsCommand class. Contributed by Daryn Sharp.
          Arun C Murthy made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development