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 tail to conform to new FsCommand class.

      1. HADOOP-7236-3.patch
        21 kB
        Daryn Sharp
      2. HADOOP-7236-2.patch
        21 kB
        Daryn Sharp
      3. HADOOP-7236.patch
        21 kB
        Daryn Sharp

        Issue Links

          Activity

          Hide
          Daryn Sharp added a comment -

          Split out mkdir command.
          Fix usage of mkdir.
          Move glob handling of PathData into PathData.java.
          Minor changes to PathData to record actual string used to instantiate object since Path does not return what it was given which breaks tests.

          Show
          Daryn Sharp added a comment - Split out mkdir command. Fix usage of mkdir. Move glob handling of PathData into PathData.java. Minor changes to PathData to record actual string used to instantiate object since Path does not return what it was given which breaks 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/12477664/HADOOP-7236.patch
          against trunk revision 1097322.

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

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

          Overall the code looks good. Minor comments -

          The mkdirs() in PathData.java file: I understand that it was added to help move FsShell to FileContext in an easier way. Since it looks like it might be out of place being in PathData.java, would it make sense to instead move it to Mkdirs.java. I understand that "fs" was moved to PathData to avoid the callers from having to deal/know about the "fs", but to me it looks better than having to call mkdirs() from PathData.java.

          In the following code:

          + stats.length == 1
          + &&
          + stats[0].getPath().equals(fs.makeQualified(globPath)))
          + {
          + // if the fq path is identical to the pattern passed, use the pattern
          + // to initialize the string value
          + items = new PathData[]

          Unknown macro: { new PathData(fs, pattern, stats[0]) }

          ;

          What happens in cases where stats.length == 1, but the globPath is just a "pattern" and not identical to the path? Would you run in to the same issue? Can you possibly use stats[0].getPath().toUri().getPath() (not pretty either) to obtain the path? Am not sure if it returns what you want it to return, but just throwing it out there....

          Very minor comment: In some places "String thePath" is used while in some other places "String thePathString" and "Path thePath". Could thePathString be used for String and thePath be used for Path?

          Show
          John George added a comment - Overall the code looks good. Minor comments - The mkdirs() in PathData.java file: I understand that it was added to help move FsShell to FileContext in an easier way. Since it looks like it might be out of place being in PathData.java, would it make sense to instead move it to Mkdirs.java. I understand that "fs" was moved to PathData to avoid the callers from having to deal/know about the "fs", but to me it looks better than having to call mkdirs() from PathData.java. In the following code: + stats.length == 1 + && + stats [0] .getPath().equals(fs.makeQualified(globPath))) + { + // if the fq path is identical to the pattern passed, use the pattern + // to initialize the string value + items = new PathData[] Unknown macro: { new PathData(fs, pattern, stats[0]) } ; What happens in cases where stats.length == 1, but the globPath is just a "pattern" and not identical to the path? Would you run in to the same issue? Can you possibly use stats [0] .getPath().toUri().getPath() (not pretty either) to obtain the path? Am not sure if it returns what you want it to return, but just throwing it out there.... Very minor comment: In some places "String thePath" is used while in some other places "String thePathString" and "Path thePath". Could thePathString be used for String and thePath be used for Path?
          Hide
          Daryn Sharp added a comment -

          The mkdirs() in PathData.java file: I understand that it was added to help move FsShell to FileContext in an easier way. Since it looks like it might be out of place being in PathData.java, would it make sense to instead move it to Mkdirs.java. I understand that "fs" was moved to PathData to avoid the callers from having to deal/know about the "fs", but to me it looks better than having to call mkdirs() from PathData.java.

          Yes, one of my end goals is greatly simplifying the switch to FileContext. I'm trying to make the FileSystem, FileContext, and/or anything else, opaque to the user of a path. This will afford us the ability to transparently use whatever is needed w/o modifying the callers when something changes.

          If it was moved to a Mkdirs.java, that would set the precedent for every fs operation being a separate class which I don't think is very desirable. Having a mkdirs(PathData) would require that method to reach into the guts to get the fs or context which defeats the purpose of "hiding" it.

          Regarding that horrible chunk of code that processes paths w/o an authority... The comment before it starts with "this is very nasty". It's entirely for backwards compatibility. If I were to do it for all paths, then other tests break. I want the initial redesign to be fully backwards compatible, then we can work on eliminating the gross inconsistencies in the code.

          Sure, I'll make the references to path/string more consistent.

          Thanks for the comments!

          Show
          Daryn Sharp added a comment - The mkdirs() in PathData.java file: I understand that it was added to help move FsShell to FileContext in an easier way. Since it looks like it might be out of place being in PathData.java, would it make sense to instead move it to Mkdirs.java. I understand that "fs" was moved to PathData to avoid the callers from having to deal/know about the "fs", but to me it looks better than having to call mkdirs() from PathData.java. Yes, one of my end goals is greatly simplifying the switch to FileContext. I'm trying to make the FileSystem, FileContext, and/or anything else, opaque to the user of a path. This will afford us the ability to transparently use whatever is needed w/o modifying the callers when something changes. If it was moved to a Mkdirs.java, that would set the precedent for every fs operation being a separate class which I don't think is very desirable. Having a mkdirs(PathData) would require that method to reach into the guts to get the fs or context which defeats the purpose of "hiding" it. Regarding that horrible chunk of code that processes paths w/o an authority... The comment before it starts with "this is very nasty". It's entirely for backwards compatibility. If I were to do it for all paths, then other tests break. I want the initial redesign to be fully backwards compatible, then we can work on eliminating the gross inconsistencies in the code. Sure, I'll make the references to path/string more consistent. Thanks for the comments!
          Hide
          Daryn Sharp added a comment -

          Rename all the "theXXX" to "XXX". Consistently use "path" and "pathString".

          Show
          Daryn Sharp added a comment - Rename all the "theXXX" to "XXX". Consistently use "path" and "pathString".
          Hide
          Hadoop QA added a comment -

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

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

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

          +1. looks good.

          Show
          John George added a comment - +1. looks good.
          Hide
          Matt Foley added a comment -

          It looks generally good, and this refactoring is very good for the FsShell.

          However, I think John George was on the right track when he questioned adding mkdirs(), etc., methods to the PathData class. We seem to already be creating FileContext as a mirror of all FS methods, we shouldn't do the same with PathData. To me it seems obvious that the invocation of mkdirs(), rightly removed from FSCommand, should be located in the Mkdir class.

          Daryl and I talked about various ways to try to "simplify the switch to FileContext". We concluded (or at least I did – sorry if I'm putting words in your mouth ) that we were going through contortions to avoid changing twenty single lines in twenty different files at some point in the future; i.e., converting the invocations of FileSystem methods to FileContext methods in each of the implementations of the various FSCommands. While it would be nice to avoid that future editing, it isn't worth complicating the design, in my opinion. And isn't it kind of the point of all this that each command (or related subset of commands) can become a separate subclass of FSCommand? Thanks.

          Show
          Matt Foley added a comment - It looks generally good, and this refactoring is very good for the FsShell. However, I think John George was on the right track when he questioned adding mkdirs(), etc., methods to the PathData class. We seem to already be creating FileContext as a mirror of all FS methods, we shouldn't do the same with PathData. To me it seems obvious that the invocation of mkdirs(), rightly removed from FSCommand, should be located in the Mkdir class. Daryl and I talked about various ways to try to "simplify the switch to FileContext". We concluded (or at least I did – sorry if I'm putting words in your mouth ) that we were going through contortions to avoid changing twenty single lines in twenty different files at some point in the future; i.e., converting the invocations of FileSystem methods to FileContext methods in each of the implementations of the various FSCommands. While it would be nice to avoid that future editing, it isn't worth complicating the design, in my opinion. And isn't it kind of the point of all this that each command (or related subset of commands) can become a separate subclass of FSCommand? Thanks.
          Hide
          Daryn Sharp added a comment -

          I agree, it felt a little awkward. My motivation was to simplify the move to FileContext or the next new and improved api. That said, you won me over, I'm fine with switching it back.

          Show
          Daryn Sharp added a comment - I agree, it felt a little awkward. My motivation was to simplify the move to FileContext or the next new and improved api. That said, you won me over, I'm fine with switching it back.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 Thanks.

          Show
          Matt Foley added a comment - +1 Thanks.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this. Thanks, Daryn!

          Also thanks John and Matt for reviewing it.

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks, Daryn! Also thanks John and Matt for reviewing it.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #571 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/571/)
          HADOOP-7236. Refactor the mkdir command to conform to new FsCommand class. Contributed by Daryn Sharp

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

          Integrated in Hadoop-Common-trunk #679 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/679/)
          HADOOP-7236. Refactor the mkdir command to conform to new FsCommand class. Contributed by Daryn Sharp

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #679 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/679/ ) HADOOP-7236 . Refactor the mkdir command 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