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:
      Reviewed

      Description

      Need to refactor ls to conform to new FsCommand class.

        Issue Links

          Activity

          Hide
          Daryn Sharp added a comment -

          Fix the usage text for ls.
          Scrub out ls code and conditionals from FsShell, move to new subclass of FsCommand.
          Add temporary "hack" to accomodate commands that don't throw the same text for a FileNotFoundException...
          Add getter/setter for the recursive flag in Command class.

          Show
          Daryn Sharp added a comment - Fix the usage text for ls. Scrub out ls code and conditionals from FsShell, move to new subclass of FsCommand. Add temporary "hack" to accomodate commands that don't throw the same text for a FileNotFoundException... Add getter/setter for the recursive flag in Command class.
          Hide
          Daryn Sharp added a comment -

          Passed test-patch, and hdfs tests.

          Show
          Daryn Sharp added a comment - Passed test-patch, and hdfs tests.
          Hide
          Hadoop QA added a comment -

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

          +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://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/368//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/368//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/368//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/12476937/HADOOP-7233.patch against trunk revision 1095121. +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://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/368//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/368//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/368//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          +1, The code looks good to me.

          One high-level comment: this refactor of FsShell is making a lot of changes and leaving a lot of TODOs in the code, all of which will be removed once the total refactor is completed. Since you're proceeding by piecemeal switching the commands over to the new system, might it make sense to do all this work in a branch, and then let us review the final product? It seems unfortunate to have transitional code in trunk over the course of this whole project, and there's the obvious question of what happens to the TODOs if the total refactor is never completed.

          Show
          Aaron T. Myers added a comment - +1, The code looks good to me. One high-level comment: this refactor of FsShell is making a lot of changes and leaving a lot of TODOs in the code, all of which will be removed once the total refactor is completed. Since you're proceeding by piecemeal switching the commands over to the new system, might it make sense to do all this work in a branch, and then let us review the final product? It seems unfortunate to have transitional code in trunk over the course of this whole project, and there's the obvious question of what happens to the TODOs if the total refactor is never completed.
          Hide
          Daryn Sharp added a comment -

          Thanks for the quick review!

          My concern with making the changes on a separate branch is the time it will take to review and commit the changes. I'm trying to avoid a "big bang" integration that will be harder for a committer, and require me to perform manual merges of any changes that occur in common and/or hdfs until it's committed. If others feel strongly, I can pursue a branch.

          Regarding the TODOs, I have added 9 in the code I have touched. They are more of a wish list than truly transitional detritus. I do intend to address most of them, but the overall task is neither at risk, nor left in a bad shape if not completed (which I am committed to completing). The breakdown is:

          • 1 is that ls should be iterative (technically out of scope for my work)
          • 1 is about how the descriptive text should be auto-wrapped (nice to have)
          • 5 are about standardizing the existing text of exceptions (nice to have)
          • 1 is about better exit codes which already has another jira (nice to have)
          • 1 is about less than ideal instantiation of the command factory, which is only an issue if dfsadmin and fsshell share a common base class instead of dfsadmin deriving from fsshell (nice to have)
          Show
          Daryn Sharp added a comment - Thanks for the quick review! My concern with making the changes on a separate branch is the time it will take to review and commit the changes. I'm trying to avoid a "big bang" integration that will be harder for a committer, and require me to perform manual merges of any changes that occur in common and/or hdfs until it's committed. If others feel strongly, I can pursue a branch. Regarding the TODOs, I have added 9 in the code I have touched. They are more of a wish list than truly transitional detritus. I do intend to address most of them, but the overall task is neither at risk, nor left in a bad shape if not completed (which I am committed to completing). The breakdown is: 1 is that ls should be iterative (technically out of scope for my work) 1 is about how the descriptive text should be auto-wrapped (nice to have) 5 are about standardizing the existing text of exceptions (nice to have) 1 is about better exit codes which already has another jira (nice to have) 1 is about less than ideal instantiation of the command factory, which is only an issue if dfsadmin and fsshell share a common base class instead of dfsadmin deriving from fsshell (nice to have)
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this. Thanks Daryn.

          Also thanks Aaron for reviewing it.

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks Daryn. Also thanks Aaron for reviewing it.
          Hide
          Aaron T. Myers added a comment -

          This was the specific code which led me to suggest that this could be better done in a branch:

          /**
          *  TODO: A crutch until the text is standardized across commands...
          *  Eventually an exception that takes the path as an argument will
          *  replace custom text
          *  @param path the thing that doesn't exist
          *  @returns String in printf format
          */
          protected String getFnfText(Path path) {
            throw new RuntimeException(path + ": No such file or directory");
          }
          

          Which obviously should be changed sooner rather than later, and arguably should never be in trunk. If the intention is that this will never actually return a String, but rather always throw an exception, then you should change the return type to be vode, and the name of the method to "throwFnfError". It should probably also throw an IOException, as we do elsewhere in the code.

          Note that, in the Hadoop projects, if you do work on a branch it's OK to do "commit-then-review" of the individual issues to speed up the development time. You'd then need to get a full review/+1 at branch merge time. I don't feel strongly about moving this work to a branch, but others might. Just a suggestion.

          Show
          Aaron T. Myers added a comment - This was the specific code which led me to suggest that this could be better done in a branch: /** * TODO: A crutch until the text is standardized across commands... * Eventually an exception that takes the path as an argument will * replace custom text * @param path the thing that doesn't exist * @returns String in printf format */ protected String getFnfText(Path path) { throw new RuntimeException(path + ": No such file or directory" ); } Which obviously should be changed sooner rather than later, and arguably should never be in trunk. If the intention is that this will never actually return a String, but rather always throw an exception, then you should change the return type to be vode , and the name of the method to " throwFnfError ". It should probably also throw an IOException, as we do elsewhere in the code. Note that, in the Hadoop projects, if you do work on a branch it's OK to do "commit-then-review" of the individual issues to speed up the development time. You'd then need to get a full review/+1 at branch merge time. I don't feel strongly about moving this work to a branch, but others might. Just a suggestion.
          Hide
          Daryn Sharp added a comment -

          Oops. I botched that, but it's never actually called (currently). The method is intended to return a string of the text for a FileNotFoundException since every command's text seems slightly different. The above base class was supposed to throw "not implemented", or return the string that got thrown. I munged it, but it's not called, so I'll fix it in the next refactored command review.

          The end goal is "throw PathNotFoundException(path)" which will enforce a standard text message, and have a getter for the path. I intend to first refactor all FsShell commands. I can then universally change them all to use the new exception with just 2 LOC, and another jira to update all the hdfs tests.

          I can see the wisdom of using a branch for larger features. Unfortunately in this case, code is being moved from a file into new files, which would make merges completely manual and error-prone. I also have someone waiting on my changes so they can add symlink support. Hence my hesitancy to use a branch...

          Show
          Daryn Sharp added a comment - Oops. I botched that, but it's never actually called (currently). The method is intended to return a string of the text for a FileNotFoundException since every command's text seems slightly different. The above base class was supposed to throw "not implemented", or return the string that got thrown. I munged it, but it's not called, so I'll fix it in the next refactored command review. The end goal is "throw PathNotFoundException(path)" which will enforce a standard text message, and have a getter for the path. I intend to first refactor all FsShell commands. I can then universally change them all to use the new exception with just 2 LOC, and another jira to update all the hdfs tests. I can see the wisdom of using a branch for larger features. Unfortunately in this case, code is being moved from a file into new files, which would make merges completely manual and error-prone. I also have someone waiting on my changes so they can add symlink support. Hence my hesitancy to use a branch...
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #561 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/561/)
          HADOOP-7233. Refactor ls to conform to new FsCommand class. Contributed by Daryn Sharp

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #561 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/561/ ) HADOOP-7233 . Refactor ls to conform to new FsCommand class. Contributed by Daryn Sharp
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #666 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/666/)
          HADOOP-7233. Refactor ls to conform to new FsCommand class. Contributed by Daryn Sharp

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #666 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/666/ ) HADOOP-7233 . Refactor ls to conform to new FsCommand class. Contributed by Daryn Sharp

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development