Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Add new file system interface AbstractFileSystem with implementation of some file systems that delegate to old FileSystem.

      Description

      The FileContext API (HADOOP-4952) provides an improved interface for the application writer.
      This lets us simplify the FileSystem API since it will no longer need to deal with notions of default filesystem [ / ], wd, and config
      defaults for blocksize, replication factor etc. Further it will not need the many overloaded methods for create() and open() since
      the FileContext API provides that convenience.
      The FileSystem API can be simplified and can now be restricted to those implementing new file systems.

      This jira proposes that we create new file system API, and deprecate FileSystem API after a few releases.

      1. AbstractFileSystem.java
        18 kB
        Sanjay Radia
      2. Hdfs.java
        4 kB
        Sanjay Radia
      3. afs1.patch
        61 kB
        Sanjay Radia
      4. Hdfs.java
        5 kB
        Sanjay Radia
      5. AFS4.patch
        94 kB
        Sanjay Radia
      6. AFS10.patch
        109 kB
        Sanjay Radia
      7. AFS11.patch
        109 kB
        Sanjay Radia
      8. AFS15.patch
        100 kB
        Sanjay Radia
      9. AFS16.patch
        109 kB
        Suresh Srinivas
      10. AFS17.patch
        113 kB
        Sanjay Radia
      11. AFS18.patch
        113 kB
        Sanjay Radia

        Issue Links

          Activity

          Hide
          Sanjay Radia added a comment -

          I propose that we call the new class AbstractFileSystem and deprecate the FileSystem class in a few releases after folks have
          migrated to FileContext.

          Note: one could deprecate parts of FileSystem and them delete or make-package private its methods. It is simpler
          to simply create a new class called AbstractFileSystem (or VirtualFileSystem a la Unix).

          Show
          Sanjay Radia added a comment - I propose that we call the new class AbstractFileSystem and deprecate the FileSystem class in a few releases after folks have migrated to FileContext. Note: one could deprecate parts of FileSystem and them delete or make-package private its methods. It is simpler to simply create a new class called AbstractFileSystem (or VirtualFileSystem a la Unix).
          Hide
          Doug Cutting added a comment -

          > one could deprecate parts of FileSystem and them delete or make-package private its methods.
          > It is simpler to simply create a new class called AbstractFileSystem

          I don't see how this is simpler. It breaks existing users and implementations just to have a different name. If we want to encourage folks to use the FileContext API in place of the FileSystem API directly then we can deprecate and subsequently remove utility methods in FileSystem that make it easier to use, so that it becomes mostly a set of abstract methods. Renaming it seems gratuitously incompatible.

          Show
          Doug Cutting added a comment - > one could deprecate parts of FileSystem and them delete or make-package private its methods. > It is simpler to simply create a new class called AbstractFileSystem I don't see how this is simpler. It breaks existing users and implementations just to have a different name. If we want to encourage folks to use the FileContext API in place of the FileSystem API directly then we can deprecate and subsequently remove utility methods in FileSystem that make it easier to use, so that it becomes mostly a set of abstract methods. Renaming it seems gratuitously incompatible.
          Hide
          Arun C Murthy added a comment -

          Renaming it seems gratuitously incompatible.

          I agree that FileSystem is the right name for the new class. However, I do see the danger of just re-purposing org.apache.hadoop.fs.FileSystem - it is quite dangerous for users who are skipping releases i.e. 0.20 to 0.22.

          Hence I propose a compromise: create a new limited-private org.apache.hadoop.vfs.FileSystem - we can start moving implementations of the FileSystem over right now. Thoughts?

          Show
          Arun C Murthy added a comment - Renaming it seems gratuitously incompatible. I agree that FileSystem is the right name for the new class. However, I do see the danger of just re-purposing org.apache.hadoop.fs.FileSystem - it is quite dangerous for users who are skipping releases i.e. 0.20 to 0.22. Hence I propose a compromise: create a new limited-private org.apache.hadoop.vfs.FileSystem - we can start moving implementations of the FileSystem over right now. Thoughts?
          Hide
          Sanjay Radia added a comment -

          Doug says:
          >I don't see how this is simpler. It breaks existing users and implementations just to have a different name.
          I am confused here. If FileSystem or parts are to be deprecated then aren't you breaking existing users anyway?
          Were you planning to allow apps to use FileContext or FileSystem forever - ie both API are for application use?
          I was proposing that new FileSystem is for implementing file systems and FileContext is for application use. (See Jira description).
          Some of the method names of the old FileSystem and new FileSystem may accidentally match but their semantics will be different:
          For example old FileSystem understands / and wd. I am proposing that the new FileSystem does not - new FileSystem will process only fully qualified names within its FileSystem; FileContext understand /, wd.
          Same class name and same method signature but different semantics across releases is confusing to users.

          I suspect we don't agree of the goal of this Jira as stated in the description. If you believe that the goal is to provide 2 layers (FileContext and FileSystem)
          one more convenient than the other but leave many of the methods in the old FileSystem compatible then I can understand why you would want to leave
          the class name the same and are surprised that I am making such a stupid suggestion to change the class name.


          Agree with Arun about the danger of re-purposing the same full class name. Either the package name or class name should change by convention.
          Although I have a preference for leaving in the fs package and calling it FileSystemBase or AbstractFileSystem, I can live with Arun's proposal (vfs.FileSystem).
          I assume that the package name vfs is derived from the unix "vfs" layer for implementing new filesystems in the Kernel.

          Also changing the name has the advantage that we can immediately create the new class (vfs.FileSystem) and change new FileContext to use it.
          My plan was to make fs.FileSystem a subclass of vfs.FileSystem, etc as soon as possible.

          Show
          Sanjay Radia added a comment - Doug says: >I don't see how this is simpler. It breaks existing users and implementations just to have a different name. I am confused here. If FileSystem or parts are to be deprecated then aren't you breaking existing users anyway? Were you planning to allow apps to use FileContext or FileSystem forever - ie both API are for application use? I was proposing that new FileSystem is for implementing file systems and FileContext is for application use. (See Jira description). Some of the method names of the old FileSystem and new FileSystem may accidentally match but their semantics will be different: For example old FileSystem understands / and wd. I am proposing that the new FileSystem does not - new FileSystem will process only fully qualified names within its FileSystem; FileContext understand /, wd. Same class name and same method signature but different semantics across releases is confusing to users. I suspect we don't agree of the goal of this Jira as stated in the description. If you believe that the goal is to provide 2 layers (FileContext and FileSystem) one more convenient than the other but leave many of the methods in the old FileSystem compatible then I can understand why you would want to leave the class name the same and are surprised that I am making such a stupid suggestion to change the class name. Agree with Arun about the danger of re-purposing the same full class name. Either the package name or class name should change by convention. Although I have a preference for leaving in the fs package and calling it FileSystemBase or AbstractFileSystem, I can live with Arun's proposal (vfs.FileSystem). I assume that the package name vfs is derived from the unix "vfs" layer for implementing new filesystems in the Kernel. Also changing the name has the advantage that we can immediately create the new class (vfs.FileSystem) and change new FileContext to use it. My plan was to make fs.FileSystem a subclass of vfs.FileSystem, etc as soon as possible.
          Hide
          Doug Cutting added a comment -

          > Some of the method names of the old FileSystem and new FileSystem may accidentally match but their semantics will be different.

          I had missed this. If that's the case, if it's really a new, incompatible API that will replace FileSystem, then a new name might be warranted. But if we're just removing some utility methods, it seems like overkill. Which of FileSystem's abstract methods would change incompatibly?

          Show
          Doug Cutting added a comment - > Some of the method names of the old FileSystem and new FileSystem may accidentally match but their semantics will be different. I had missed this. If that's the case, if it's really a new, incompatible API that will replace FileSystem, then a new name might be warranted. But if we're just removing some utility methods, it seems like overkill. Which of FileSystem's abstract methods would change incompatibly?
          Hide
          Sanjay Radia added a comment -

          Turns out most of them wrt to the semantics: all methods that have a path name are affected since the path name
          in the new FS is a fully qualified name and will throw an exception if it is not. Also those with the permissions are
          affected - the new FS takes in absolute permissions (ie no need to apply the mask).
          In both cases the FileContext class will convert relative names into fully qualified and apply the mask to the perm before calling the
          new FS.
          I am still debating where the path name passed should be fully qualified URI (but checked to be in the same FS) or FS-root relative
          path in which case it does not need to check - from a spec point of view the latter is cleaner. (Do we have type for a root relative path?)

          Show
          Sanjay Radia added a comment - Turns out most of them wrt to the semantics: all methods that have a path name are affected since the path name in the new FS is a fully qualified name and will throw an exception if it is not. Also those with the permissions are affected - the new FS takes in absolute permissions (ie no need to apply the mask). In both cases the FileContext class will convert relative names into fully qualified and apply the mask to the perm before calling the new FS. I am still debating where the path name passed should be fully qualified URI (but checked to be in the same FS) or FS-root relative path in which case it does not need to check - from a spec point of view the latter is cleaner. (Do we have type for a root relative path?)
          Hide
          Sanjay Radia added a comment -

          Migration strategy:
          Two options here:

          1. Transition by delegating from FileSystem to the new AbstractFileSystem (possibly over one more more releases)
            • Add FileContext; FileContext calls FileSystem
            • Deprecate FileSystem
            • Add AbstractFileSystem
            • Migrate each FS-impl to AbstractFileSystem and use a FileSystem delegator to delegate FileSystem calls to AbstractFileSystem so that AFS-impl is called. This step may occur over one or more releases.
            • Change FileContext to call AbstractFileSystem
            • Remove FileSystem (say after 1 or 2 releases)
          2. Transition by delegating from AbstractFileSystem to the new FileSystem (possibly over one more more releases)
            • Add FileContext and AbstractFileSystem; FileContext calls AbsrtactFileSystem
            • AbstractFileSystem delegates to FileSystem for each AFS-impl that has not yet been implemented
            • Migrate each FS-Impl to AbstractFileSystem -the delegator will no longer call FS-Impl, instead AFS_impl will be called directly. This step may occur over one or more releases.
            • Remove FileSystem (say after 1 or 2 releases)

          Originally I was planning to use option 1 and hence my patch for FileContext calls FileSystem.
          I am seriously considering going with option 2.

          Show
          Sanjay Radia added a comment - Migration strategy: Two options here: Transition by delegating from FileSystem to the new AbstractFileSystem (possibly over one more more releases) Add FileContext; FileContext calls FileSystem Deprecate FileSystem Add AbstractFileSystem Migrate each FS-impl to AbstractFileSystem and use a FileSystem delegator to delegate FileSystem calls to AbstractFileSystem so that AFS-impl is called. This step may occur over one or more releases. Change FileContext to call AbstractFileSystem Remove FileSystem (say after 1 or 2 releases) Transition by delegating from AbstractFileSystem to the new FileSystem (possibly over one more more releases) Add FileContext and AbstractFileSystem; FileContext calls AbsrtactFileSystem AbstractFileSystem delegates to FileSystem for each AFS-impl that has not yet been implemented Migrate each FS-Impl to AbstractFileSystem -the delegator will no longer call FS-Impl, instead AFS_impl will be called directly. This step may occur over one or more releases. Remove FileSystem (say after 1 or 2 releases) Originally I was planning to use option 1 and hence my patch for FileContext calls FileSystem. I am seriously considering going with option 2.
          Hide
          Doug Cutting added a comment -

          > all methods that have a path name are affected since the path name
          > in the new FS is a fully qualified name and will throw an exception if it is not.

          So the concern is that if you remove from each FileSystem implementation its ability to handle non-fully-qualified paths then existing applications which don't switch to FileContext and which pass non-fully-qualified paths may fail, right? That makes sense.

          > I am seriously considering going with option 2.

          Can you seriously explain why?

          Show
          Doug Cutting added a comment - > all methods that have a path name are affected since the path name > in the new FS is a fully qualified name and will throw an exception if it is not. So the concern is that if you remove from each FileSystem implementation its ability to handle non-fully-qualified paths then existing applications which don't switch to FileContext and which pass non-fully-qualified paths may fail, right? That makes sense. > I am seriously considering going with option 2. Can you seriously explain why?
          Hide
          Todd Lipcon added a comment -

          Ach, commented on the wrong JIRA. The above comment was supposed to go on the related JIRA HADOOP-4952. If someone has "delete" access on JIRA feel free to delete the above lengthy comment.

          Show
          Todd Lipcon added a comment - Ach, commented on the wrong JIRA. The above comment was supposed to go on the related JIRA HADOOP-4952 . If someone has "delete" access on JIRA feel free to delete the above lengthy comment.
          Hide
          Sanjay Radia added a comment -

          @Doug:
          > I am seriously considering going with option 2.
          >>Can you seriously explain why?
          Mostly it is personal choice but my reasons are:

          1. Since option 2 is the final state it is generally good to start with that.
          2. Forces one to design the AbstractFileSystem - FileContext relationship better. I had to keep the difference between old FileSystem and the new AbstractFileSystem clear in my head. Comments from a few folks also confused this distinction: E.g. several folks commented "getInitialWorkingDir() is not needed since we have getWd'; in spite of my comment in the getInitalWD() it wasn't clear that the new FileSystem, is not suppose to know about wd - it can have an opinion only on the initial wd.
          3. Both codepaths
            • Applications to FileContext to AbstractFileSystem to AbstractFileSystemImpls and
            • Applications to FileSystem to FileSystemImpls

          are part of the system from the beginning. In option 2 the flip at the end introduces the final code path. This is better for testing.

          Show
          Sanjay Radia added a comment - @Doug: > I am seriously considering going with option 2. >>Can you seriously explain why? Mostly it is personal choice but my reasons are: Since option 2 is the final state it is generally good to start with that. Forces one to design the AbstractFileSystem - FileContext relationship better. I had to keep the difference between old FileSystem and the new AbstractFileSystem clear in my head. Comments from a few folks also confused this distinction: E.g. several folks commented "getInitialWorkingDir() is not needed since we have getWd'; in spite of my comment in the getInitalWD() it wasn't clear that the new FileSystem, is not suppose to know about wd - it can have an opinion only on the initial wd. Both codepaths Applications to FileContext to AbstractFileSystem to AbstractFileSystemImpls and Applications to FileSystem to FileSystemImpls are part of the system from the beginning. In option 2 the flip at the end introduces the final code path. This is better for testing.
          Hide
          Doug Cutting added a comment -

          On HADOOP-6240 Sanjay says, "I have gone for option 1 rather than 2". I thought you preferred option 2. HADOOP-6240 makes the case for option 2. Why have you abandoned it?

          Show
          Doug Cutting added a comment - On HADOOP-6240 Sanjay says, "I have gone for option 1 rather than 2". I thought you preferred option 2. HADOOP-6240 makes the case for option 2. Why have you abandoned it?
          Hide
          Sanjay Radia added a comment -

          I am amazed by your comment above! I would have preferred to have this discussion in private; but your comment leaves me with little choice:
          You first objected to the notion of having a different class and hence the need for this Jira.
          Owen had to step in with a private discussion with you to get you see the other side of the story (which was well explained in this and the related filecontext jira). While you were having that discussion with Owen we had concluded that this jira is likely to be caught up in a debate and charged full speed ahead in doing FileContext without the the proposed AbstractFileContext.
          Furthermore, you later questioned my proposal to go with option 2 anyway and hence implicitly suggested that you prefer that FileContext call FileSystem and not AbstractFileContext in 21.
          I needed to move forward. Moving forward with option 2 was risky till I convinced you.
          In either case, I was still planning to add the create2 and rename2 methods to FileSystem (because it offered simpler and better delegation choices)
          which you would have objected to anyway - so the debate we are having wrt to rename2 method in 6240 would have happened anyway.
          Making progress on some of these Jiras is very hard because you look at only bits and pieces of the puzzle
          and have very strong opinions on style. The design choices I had made were reasonable and discussed extensively with owen, arun, chris, suresh, and others. The FileContext patch has also articulated the design quite well. You just have a different opinion. Luckily it seems to to overlap (though not completely) with my option 2 above and so we can move forward.
          I will sketch a plan for option 2 and post it to this or the filecontext jira tomorrow.

          I hope I have not offended you or others in my comments above. We are under a lot of pressure to get this compatibility stuff I launched a year ago to be completed in release 21 which freezes next week.
          I feel it is best to be frank and help you see the other side of the story; I hope we can make faster progress as a result.

          Show
          Sanjay Radia added a comment - I am amazed by your comment above! I would have preferred to have this discussion in private; but your comment leaves me with little choice: You first objected to the notion of having a different class and hence the need for this Jira. Owen had to step in with a private discussion with you to get you see the other side of the story (which was well explained in this and the related filecontext jira). While you were having that discussion with Owen we had concluded that this jira is likely to be caught up in a debate and charged full speed ahead in doing FileContext without the the proposed AbstractFileContext. Furthermore, you later questioned my proposal to go with option 2 anyway and hence implicitly suggested that you prefer that FileContext call FileSystem and not AbstractFileContext in 21. I needed to move forward. Moving forward with option 2 was risky till I convinced you. In either case, I was still planning to add the create2 and rename2 methods to FileSystem (because it offered simpler and better delegation choices) which you would have objected to anyway - so the debate we are having wrt to rename2 method in 6240 would have happened anyway. Making progress on some of these Jiras is very hard because you look at only bits and pieces of the puzzle and have very strong opinions on style. The design choices I had made were reasonable and discussed extensively with owen, arun, chris, suresh, and others. The FileContext patch has also articulated the design quite well. You just have a different opinion. Luckily it seems to to overlap (though not completely) with my option 2 above and so we can move forward. I will sketch a plan for option 2 and post it to this or the filecontext jira tomorrow. I hope I have not offended you or others in my comments above. We are under a lot of pressure to get this compatibility stuff I launched a year ago to be completed in release 21 which freezes next week. I feel it is best to be frank and help you see the other side of the story; I hope we can make faster progress as a result.
          Hide
          Doug Cutting added a comment -

          > You first objected to the notion of having a different class and hence the need for this Jira.

          I questioned it, as it didn't initially seem well motivated. The changes in semantics of same-named methods subsequently provided a compelling motivation. That argument was not in the initial description.

          > Making progress on some of these Jiras is very hard because you look at only bits and pieces of the puzzle [ ...]

          The puzzle needs to be presented clearly to all, not in private discussions. I have monitored all related Jira issues, and responded promptly to comments in these. Your motivations were not clear to me until I asked some questions. Now I can see the case for AbstractFileSystem and your option 2. I do not yet see a case for rename2.

          > The design choices I had made were reasonable and discussed extensively with owen, arun, chris, suresh, and others.

          Can you point to these discussions? Co-location is an open-source anti-pattern. Outsiders cannot just assume that every proposal of yours is perfect. Proposals must succeed on merit. I scrutinize proposals that change fundamental public APIs much more closely than other proposals. Deprecating FileSystem is a big deal and should be done very carefully and with community involvement.

          Show
          Doug Cutting added a comment - > You first objected to the notion of having a different class and hence the need for this Jira. I questioned it, as it didn't initially seem well motivated. The changes in semantics of same-named methods subsequently provided a compelling motivation. That argument was not in the initial description. > Making progress on some of these Jiras is very hard because you look at only bits and pieces of the puzzle [ ...] The puzzle needs to be presented clearly to all, not in private discussions. I have monitored all related Jira issues, and responded promptly to comments in these. Your motivations were not clear to me until I asked some questions. Now I can see the case for AbstractFileSystem and your option 2. I do not yet see a case for rename2. > The design choices I had made were reasonable and discussed extensively with owen, arun, chris, suresh, and others. Can you point to these discussions? Co-location is an open-source anti-pattern. Outsiders cannot just assume that every proposal of yours is perfect. Proposals must succeed on merit. I scrutinize proposals that change fundamental public APIs much more closely than other proposals. Deprecating FileSystem is a big deal and should be done very carefully and with community involvement.
          Hide
          Sanjay Radia added a comment -

          @Doug >I do not yet see a case for rename2.
          I understand that Owen presented to you the case for rename2 and option 1 in private discussions last week and that you were not convinced.

          Let me summarize the case for option 1 and rename 2 for the benefit of the rest of the community.
          Please refer to the options 1 and options 2 above in this Jira. Further note that both options get us to the same end state: a new parallel stack where applications call FileContext which in turn calls AFS-impls. Option 1 uses the existing FileSystem APi and implementation in the early phase as we migrate from old stack to new stack. As I have mentioned above I have been going with option1 instead of 2; my patches in HADOOP-4952 have been based on option 1. I did strongly consider option 2 but felt that it raised the risk in this project (details below).

          Does the FileSystem API have to be enhanced to support FileContext?
          Yes. If you look at the patches for FileContext (HADOOP-4952) they have added 3 protected methods: getInitialWorkingDir(), createAbsPerm() mkdirsAbsPerm() (btw in the latest patch the last two methods were renamed to primitveCreate() and primitiveMkdirs().
          These 3 methods were all declared protected and hence not visible to the applications. Once we have the full new stack, these methods can be deleted.

          What has this got to do with rename2()?
          Turns out that our rename implementation is broken. Fixing the FileSystem#rename spec would potentially break applications. Given that we are introducing a new fs api (FileContext) it has been proposed that we leave the old FileSystem#rename & its spec as is and simply add a new protected method FileSystem#rename2() - its sole purpose is to support FileContext#rename like the other 3 protected methods mentioned above.

          Why did you choose to go with Option 1 and not option 2.
          Option 1 was easier to get started because it leveraged existing FileSystem to the fullest. AFS on the other hand was debated as as soon as it started and further the option 2 was questioned. I felt that the community needed some time to digest this Jira. Comments from 3 folks is very little in contrast to the large number of comments in FileContext Jira. Further, my intuition told me that there were a number details to be resolved. The FileSystem design and implementation are very messy and I didn't want to simply carry forward its design without debate.

          Over the weekend, as I explored option 2 , my intuition was correct: here is a list of issues to be resolved for AFS. While none of them are impossible to solve, they are not trivial either.

          • where should the cache go? In FC or AFS.? Is the cache keyed off the config or not (the cache is FS seems to be somewhat tied to the config. - I think we need to look at that closely). The cache has leaked through the FileSytem API - I would like to avoid that for AFS.
          • Delete-on-exit - should we raise it to FC or leave it in AFS. There are certain assumptions made by the current delete-on-exit that seem incorrect and should be revisited.
            • What do we do about the public close method?
          • Statistics features in FS. - where does it go in the new world.
            Given the above, I had felt it was wiser to go with option 1 since its only cost is a few protected methods. Further, even in option 2 these protected methods would have helped us would have simplified delegation from AFS to FileSystem.

          It had always been my goal that as soon as the FileContext was committed I would complete this AFS jira and perhaps even switch from option 1 to option 2 midway if there was sufficient time.

          So far I don't understand the objections to option1 (and to rename2) ; protected methods seems reasonable in this situation. Is this a style issue? If the objections are minor I feel it is better to give this AFS jira sufficient time for community discussion and go with option 1. If there are serious objections to Option 1 then by all means lets put all the wood behind the option 2 arrow.

          BTW Option 1 would have been completed by this Friday according to our original plan. Option 2 will not be completed by the freeze date on Friday but we have started work on it.

          Show
          Sanjay Radia added a comment - @Doug >I do not yet see a case for rename2. I understand that Owen presented to you the case for rename2 and option 1 in private discussions last week and that you were not convinced. Let me summarize the case for option 1 and rename 2 for the benefit of the rest of the community. Please refer to the options 1 and options 2 above in this Jira. Further note that both options get us to the same end state : a new parallel stack where applications call FileContext which in turn calls AFS-impls. Option 1 uses the existing FileSystem APi and implementation in the early phase as we migrate from old stack to new stack. As I have mentioned above I have been going with option1 instead of 2; my patches in HADOOP-4952 have been based on option 1. I did strongly consider option 2 but felt that it raised the risk in this project (details below). Does the FileSystem API have to be enhanced to support FileContext? Yes. If you look at the patches for FileContext ( HADOOP-4952 ) they have added 3 protected methods: getInitialWorkingDir(), createAbsPerm() mkdirsAbsPerm() (btw in the latest patch the last two methods were renamed to primitveCreate() and primitiveMkdirs(). These 3 methods were all declared protected and hence not visible to the applications. Once we have the full new stack, these methods can be deleted. What has this got to do with rename2()? Turns out that our rename implementation is broken. Fixing the FileSystem#rename spec would potentially break applications. Given that we are introducing a new fs api (FileContext) it has been proposed that we leave the old FileSystem#rename & its spec as is and simply add a new protected method FileSystem#rename2() - its sole purpose is to support FileContext#rename like the other 3 protected methods mentioned above. Why did you choose to go with Option 1 and not option 2. Option 1 was easier to get started because it leveraged existing FileSystem to the fullest. AFS on the other hand was debated as as soon as it started and further the option 2 was questioned. I felt that the community needed some time to digest this Jira. Comments from 3 folks is very little in contrast to the large number of comments in FileContext Jira. Further, my intuition told me that there were a number details to be resolved. The FileSystem design and implementation are very messy and I didn't want to simply carry forward its design without debate. Over the weekend, as I explored option 2 , my intuition was correct: here is a list of issues to be resolved for AFS. While none of them are impossible to solve, they are not trivial either. where should the cache go? In FC or AFS.? Is the cache keyed off the config or not (the cache is FS seems to be somewhat tied to the config. - I think we need to look at that closely). The cache has leaked through the FileSytem API - I would like to avoid that for AFS. Delete-on-exit - should we raise it to FC or leave it in AFS. There are certain assumptions made by the current delete-on-exit that seem incorrect and should be revisited. • What do we do about the public close method? Statistics features in FS. - where does it go in the new world. Given the above, I had felt it was wiser to go with option 1 since its only cost is a few protected methods. Further, even in option 2 these protected methods would have helped us would have simplified delegation from AFS to FileSystem. It had always been my goal that as soon as the FileContext was committed I would complete this AFS jira and perhaps even switch from option 1 to option 2 midway if there was sufficient time. So far I don't understand the objections to option1 (and to rename2) ; protected methods seems reasonable in this situation. Is this a style issue? If the objections are minor I feel it is better to give this AFS jira sufficient time for community discussion and go with option 1. If there are serious objections to Option 1 then by all means lets put all the wood behind the option 2 arrow. BTW Option 1 would have been completed by this Friday according to our original plan. Option 2 will not be completed by the freeze date on Friday but we have started work on it.
          Hide
          Sanjay Radia added a comment -

          Proposed package and file system naming convention.
          AbstractFileSystem and FileContext remain in the fs package.
          AFS impls all end with fs (we would have prefered Filesystem but that is taken).

          fs

          • FileContext
          • AbstractFileSystem
          • Filterfs
          • fs.hdfs
            • Hdfs
          • fs.ftp
            • Ftpfs
          • fs.kfs
            • Kfs
          • fs.local
            • Localfs
            • RawLocalfs
          • fs.s3
            • S3fs
          • fs.s3native
            • S3nativefs
          • fs.shell
          Show
          Sanjay Radia added a comment - Proposed package and file system naming convention. AbstractFileSystem and FileContext remain in the fs package. AFS impls all end with fs (we would have prefered Filesystem but that is taken). fs FileContext AbstractFileSystem Filterfs fs.hdfs Hdfs fs.ftp Ftpfs fs.kfs Kfs fs.local Localfs RawLocalfs fs.s3 S3fs fs.s3native S3nativefs fs.shell
          Hide
          Doug Cutting added a comment -

          > AFS impls all end with fs

          I'd generally prefer "Fs".

          When acronyms are incorporated in Java names there are two styles: all caps or capitalized. I prefer capitalized (e.g., FooUrl versus FooURL). I have also seen this recommended in several Java style guides and never the contrary, e.g.:

          http://blogs.sun.com/tor/entry/code_advice_3_don_t
          http://www.burlesontech.com/wiki/display/btg/Java+Naming+Standards+and+Guidelines
          http://stackoverflow.com/questions/1176950/acronyms-in-camel-back/1177184
          etc.

          FTP is an independent acronym, so FtpFs is probably best.
          HD and K are not independent acronyms, so "Hdfs" and "Kfs" are probably preferred.
          The rest are simpler: FilterFs, LocalFs, RawLocalFs, S3Fs, & S3NativeFs.

          Show
          Doug Cutting added a comment - > AFS impls all end with fs I'd generally prefer "Fs". When acronyms are incorporated in Java names there are two styles: all caps or capitalized. I prefer capitalized (e.g., FooUrl versus FooURL). I have also seen this recommended in several Java style guides and never the contrary, e.g.: http://blogs.sun.com/tor/entry/code_advice_3_don_t http://www.burlesontech.com/wiki/display/btg/Java+Naming+Standards+and+Guidelines http://stackoverflow.com/questions/1176950/acronyms-in-camel-back/1177184 etc. FTP is an independent acronym, so FtpFs is probably best. HD and K are not independent acronyms, so "Hdfs" and "Kfs" are probably preferred. The rest are simpler: FilterFs, LocalFs, RawLocalFs, S3Fs, & S3NativeFs.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          >> AFS impls all end with fs
          >
          >I'd generally prefer "Fs".
          Fs is better. AFS is taken by Andrew File System.

          Filed HADOOP-4357 sometime ago for renaming the existing classes. Still do not have a chance to work on it.

          Show
          Tsz Wo Nicholas Sze added a comment - >> AFS impls all end with fs > >I'd generally prefer "Fs". Fs is better. AFS is taken by Andrew File System. Filed HADOOP-4357 sometime ago for renaming the existing classes. Still do not have a chance to work on it.
          Hide
          Jakob Homan added a comment -

          Rather than AbstractFileSystem.java, just Fs.java may be better since it (1) follows typical naming convention for an abstract class and its implementing classes, (2) doesn't conflict with Andrew File System and (3) an interface should be indistinguishable from the implementation, particularly in name.

          Show
          Jakob Homan added a comment - Rather than AbstractFileSystem.java, just Fs.java may be better since it (1) follows typical naming convention for an abstract class and its implementing classes, (2) doesn't conflict with Andrew File System and (3) an interface should be indistinguishable from the implementation, particularly in name.
          Hide
          dhruba borthakur added a comment -

          Fs looks good to me too.

          Show
          dhruba borthakur added a comment - Fs looks good to me too.
          Hide
          Sanjay Radia added a comment -

          Sorry made mistake of making it subtask.

          Show
          Sanjay Radia added a comment - Sorry made mistake of making it subtask.
          Hide
          Arun C Murthy added a comment -

          I'm not a fan of using abbreviations for class-names... I'd like to reiterate my previous proposal:

          limited-private org.apache.hadoop.vfs.FileSystem

          Thoughts?

          Show
          Arun C Murthy added a comment - I'm not a fan of using abbreviations for class-names... I'd like to reiterate my previous proposal: limited-private org.apache.hadoop.vfs.FileSystem Thoughts?
          Hide
          Sanjay Radia added a comment -

          I have attached a draft for AbstractFileSystem and one impl Hdfs.
          Will add a delegator class that delegates calls to old Filesystem for all the other filesystems (local fs, s3, kfs etc).

          Q. Strictly speaking AbstractFileSystem should take a slash-relative name or or Fully-qualified-uri
          that is for the this filesystem.
          Should we restrict AbstractFileSystem to require fully-qualified pathnames? My current impl does that to
          help me catch bugs in the upper layer (FileContext) that accidently sends a slash-path name to the wrong file system.
          (The glob code btw had such a bug.)

          Show
          Sanjay Radia added a comment - I have attached a draft for AbstractFileSystem and one impl Hdfs. Will add a delegator class that delegates calls to old Filesystem for all the other filesystems (local fs, s3, kfs etc). Q. Strictly speaking AbstractFileSystem should take a slash-relative name or or Fully-qualified-uri that is for the this filesystem. Should we restrict AbstractFileSystem to require fully-qualified pathnames? My current impl does that to help me catch bugs in the upper layer (FileContext) that accidently sends a slash-path name to the wrong file system. (The glob code btw had such a bug.)
          Hide
          Sanjay Radia added a comment -

          Subject: Evolution of APIs: adding new arguments to create and rename in the future.

          Take a look at AbstractFileSystem#create and AbstractFileSystem#createInternal.

          The signature is
          createInternal(final Path f, final EnumSet<CreateFlag> createFlag, CreateOpts... opts).

          Original thought was that as we evolve and add new parameters the lower layers can ignore the new arguments.

          This only works if the new arg is optional.
          If the newly is manadatory then this does not work.

          There 3 choices here

          1. ignore the problem - we will never add new mandatory args in the future
          2. add overloaded methods in the future as we add new args (classic approach)
          3. let the filesystem-impl throw unknownOpt with a list of unknown opts and have the default impl provide the required behaviour.

          So choice 2 will be the familiar classic approach

          final create(f, flag, CreateOpts ...opt) {
             // check arg validity
            // extract the arguments out of opts and add defaults for the missing ones
            createInternal(f, flag, permission, blocksize, checkSumSize, replication, iobuffersize, progress);
          }
          
          abstract createInternal(f, flag, permission, blocksize, checkSumSize, replication, iobuffersize, progress);
          

          later when we add a new fooArg:

          // filesystem-impl can override this if they desire better behaviour.
          createInternal(f, flag, permission, blocksize, checkSumSize, replication, iobuffersize, progress, fooArg) {
               // provide default behavious for argFoo
            createInternal(f, flag, permission, blocksize, checkSumSize, replication, iobuffersize, progress);
          }
          

          Choice 3 will look like:

          final create(f, flag, CreateOpts ...opt) {
             // check arg validity
          
            try {
            createInternal(f, flag, opt);
           } catch (e) {
             unknownOpt {
               // take the list of unknown options  and provide the default behaviour.
               // we are will have to deal with  default  impl for all new mandatory options
             } 
          }
          
          abstract createInternal((f, flag, CreateOpts ...opt);
          
          Show
          Sanjay Radia added a comment - Subject: Evolution of APIs: adding new arguments to create and rename in the future. Take a look at AbstractFileSystem#create and AbstractFileSystem#createInternal. The signature is createInternal(final Path f, final EnumSet<CreateFlag> createFlag, CreateOpts... opts). Original thought was that as we evolve and add new parameters the lower layers can ignore the new arguments. This only works if the new arg is optional. If the newly is manadatory then this does not work. There 3 choices here ignore the problem - we will never add new mandatory args in the future add overloaded methods in the future as we add new args (classic approach) let the filesystem-impl throw unknownOpt with a list of unknown opts and have the default impl provide the required behaviour. So choice 2 will be the familiar classic approach final create(f, flag, CreateOpts ...opt) { // check arg validity // extract the arguments out of opts and add defaults for the missing ones createInternal(f, flag, permission, blocksize, checkSumSize, replication, iobuffersize, progress); } abstract createInternal(f, flag, permission, blocksize, checkSumSize, replication, iobuffersize, progress); later when we add a new fooArg: // filesystem-impl can override this if they desire better behaviour. createInternal(f, flag, permission, blocksize, checkSumSize, replication, iobuffersize, progress, fooArg) { // provide default behavious for argFoo createInternal(f, flag, permission, blocksize, checkSumSize, replication, iobuffersize, progress); } Choice 3 will look like: final create(f, flag, CreateOpts ...opt) { // check arg validity try { createInternal(f, flag, opt); } catch (e) { unknownOpt { // take the list of unknown options and provide the default behaviour. // we are will have to deal with default impl for all new mandatory options } } abstract createInternal((f, flag, CreateOpts ...opt);
          Hide
          Sanjay Radia added a comment -

          >Fs is better. AFS is taken by Andrew File System.
          The class name is AbstractFileSystem; I was just using AFS in my comments because I was too lazy to type the full class name.
          I am open to other names Fs, Vfs, FileSystemBase, etc.

          Show
          Sanjay Radia added a comment - >Fs is better. AFS is taken by Andrew File System. The class name is AbstractFileSystem; I was just using AFS in my comments because I was too lazy to type the full class name. I am open to other names Fs, Vfs, FileSystemBase, etc.
          Hide
          Sanjay Radia added a comment -

          Updated patch for AbstractFileSystem.
          Includes a delegator to old FileSystem, a delegated implementation of LocalFs and impls of Hdfs, ChecksumFs, FilterFs.

          Note wrt to my above comment on API evolution, create() uses option 3 while rename() uses option 2.
          Am heavily leaning towards option 2 (classic approach).

          This is getting fairly close to be being done. Please review and give feedback.

          Show
          Sanjay Radia added a comment - Updated patch for AbstractFileSystem. Includes a delegator to old FileSystem, a delegated implementation of LocalFs and impls of Hdfs, ChecksumFs, FilterFs. Note wrt to my above comment on API evolution, create() uses option 3 while rename() uses option 2. Am heavily leaning towards option 2 (classic approach). This is getting fairly close to be being done. Please review and give feedback.
          Hide
          Konstantin Shvachko added a comment -

          I definitely prefer choice 2 over choice 3. Choice 3 is more convenient for implementation in case of API evolution, but it does not qualify as an API strictly speaking, because it does not tell you which parameters it needs to do a create or rename.
          An exaggerated example of a choice 3 API would be replacing all abstract methods in AbstractFileSystem by one

          abstract void processOp(op, void ... options);
          

          where you list all possible arguments in options.
          This is very convenient for keeping different fs implementations independent of each other, but it hides the definition of the API inside the implementation of processOp() while we want it to be declared in the signature of of each method.
          CreateOpts should combine some simple flags like OVERWRITE, APPEND, etc.

          Show
          Konstantin Shvachko added a comment - I definitely prefer choice 2 over choice 3. Choice 3 is more convenient for implementation in case of API evolution, but it does not qualify as an API strictly speaking, because it does not tell you which parameters it needs to do a create or rename. An exaggerated example of a choice 3 API would be replacing all abstract methods in AbstractFileSystem by one abstract void processOp(op, void ... options); where you list all possible arguments in options . This is very convenient for keeping different fs implementations independent of each other, but it hides the definition of the API inside the implementation of processOp() while we want it to be declared in the signature of of each method. CreateOpts should combine some simple flags like OVERWRITE, APPEND, etc.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          In the current implementation, if an option is specified but the underlying FileSystem is not supported (e.g. specifying replication on LocalFileSystem), the specified option will be ignored silently. This seems to be hard to understand from users' point of view. I think we should clarify the options on the javadoc/user guide.

          Show
          Tsz Wo Nicholas Sze added a comment - In the current implementation, if an option is specified but the underlying FileSystem is not supported (e.g. specifying replication on LocalFileSystem), the specified option will be ignored silently. This seems to be hard to understand from users' point of view. I think we should clarify the options on the javadoc/user guide.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12421858/AFS10.patch
          against trunk revision 823756.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 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 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.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/82/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/82/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/82/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/82/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/12421858/AFS10.patch against trunk revision 823756. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 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 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. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/82/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/82/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/82/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/82/console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12421928/AFS11.patch
          against trunk revision 824516.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 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 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.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/84/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/84/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/84/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/84/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/12421928/AFS11.patch against trunk revision 824516. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 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 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. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/84/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/84/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/84/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/84/console This message is automatically generated.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          The javadoc warnings can be fixed by replacing

             *  {@link #create(Path, EnumSet, CreateOpts...)}
          

          with

             *  {@link #create(Path, EnumSet, Options.CreateOpts...)}
          

          javadoc does not handle inner class well.

          Show
          Tsz Wo Nicholas Sze added a comment - The javadoc warnings can be fixed by replacing * {@link #create(Path, EnumSet, CreateOpts...)} with * {@link #create(Path, EnumSet, Options.CreateOpts...)} javadoc does not handle inner class well.
          Hide
          Suresh Srinivas added a comment -

          Comments:

          1. For all the new classes added, need to add Interface classification, audience and stability.
          2. AbstractFileSystem.getMyUri() - remove this and use getUri()
          3. AbstractFileSystem should be InterfaceAudience.Public right instead of Private?

          This goes only into trunk right?

          Nits:

          1. AbstractFileSystem.java - class comment "interface to for implementors" to "interface for implementors"
          2. AbstractFileSystem.java - spell check words, move member variables to the top of the class
          3. AbstractFileSystem.java - format method comments no to include extra indentation.
          4. DelegateToFileSystem - add comments to describe the class
          Show
          Suresh Srinivas added a comment - Comments: For all the new classes added, need to add Interface classification, audience and stability. AbstractFileSystem.getMyUri() - remove this and use getUri() AbstractFileSystem should be InterfaceAudience.Public right instead of Private? This goes only into trunk right? Nits: AbstractFileSystem.java - class comment "interface to for implementors" to "interface for implementors" AbstractFileSystem.java - spell check words, move member variables to the top of the class AbstractFileSystem.java - format method comments no to include extra indentation. DelegateToFileSystem - add comments to describe the class
          Hide
          Sanjay Radia added a comment -

          Updated patch that address the comments so far.

          Show
          Sanjay Radia added a comment - Updated patch that address the comments so far.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422261/AFS15.patch
          against trunk revision 826141.

          +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 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.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/94/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/94/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/94/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/94/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/12422261/AFS15.patch against trunk revision 826141. +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 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. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/94/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/94/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/94/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/94/console This message is automatically generated.
          Hide
          Suresh Srinivas added a comment -

          Uploading a patch that includes two missing files.

          Show
          Suresh Srinivas added a comment - Uploading a patch that includes two missing files.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422657/AFS16.patch
          against trunk revision 826141.

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

          +1 tests included. The patch appears to include 9 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 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.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/95/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/95/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/95/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/95/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/12422657/AFS16.patch against trunk revision 826141. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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 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. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/95/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/95/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/95/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/95/console This message is automatically generated.
          Hide
          Suresh Srinivas added a comment -

          Comments:

          1. AFS - move static member variables to the beginning of the class
          2. AFS.FTP_SCHEME should move to FtpFS. AFS.LOCAL_FS_URI should move to RawLocalFs or some constants file.
          3. AFS.isValidName - not sure why check for "/" is being done, given that tokenizer uses it as separator?
          4. AFS statistics - file a jira to port FileSystem tests including statistics related tests to test AFS.
          5. AFS.get() - remove check for null scheme since it is done in AbstractFileSystem constructor. For authority, we could even have a flag needAuthority that can be passed by the implementations to AbstractFileSystem constructor.
          6. AFS.getPathPart() is getUriPath a better name for this method?
          7. AFS.statistics should be protected or method AFS.getStatistics() should be protected for subclasses to set stats.
          8. Underlying FileSystem.statistics should be same as AFS.statistics.
          9. FilterFs.getUriDefaultPort() should delegate the call to underlying AFS. With this LocalFs need not override this method.
          10. FilterFs.myFs should this or getMyFs() be protected to allow subclass to access it.
          Show
          Suresh Srinivas added a comment - Comments: AFS - move static member variables to the beginning of the class AFS.FTP_SCHEME should move to FtpFS. AFS.LOCAL_FS_URI should move to RawLocalFs or some constants file. AFS.isValidName - not sure why check for "/" is being done, given that tokenizer uses it as separator? AFS statistics - file a jira to port FileSystem tests including statistics related tests to test AFS. AFS.get() - remove check for null scheme since it is done in AbstractFileSystem constructor. For authority, we could even have a flag needAuthority that can be passed by the implementations to AbstractFileSystem constructor. AFS.getPathPart() is getUriPath a better name for this method? AFS.statistics should be protected or method AFS.getStatistics() should be protected for subclasses to set stats. Underlying FileSystem.statistics should be same as AFS.statistics. FilterFs.getUriDefaultPort() should delegate the call to underlying AFS. With this LocalFs need not override this method. FilterFs.myFs should this or getMyFs() be protected to allow subclass to access it.
          Hide
          Sanjay Radia added a comment -

          Attached patch addresses Suresh's comments

          Show
          Sanjay Radia added a comment - Attached patch addresses Suresh's comments
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12423587/AFS17.patch
          against trunk revision 829289.

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

          +1 tests included. The patch appears to include 9 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 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.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/113/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/113/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/113/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/113/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/12423587/AFS17.patch against trunk revision 829289. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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 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. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/113/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/113/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/113/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/113/console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12423627/AFS18.patch
          against trunk revision 831070.

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

          +1 tests included. The patch appears to include 9 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 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.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/115/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/115/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/115/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/115/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/12423627/AFS18.patch against trunk revision 831070. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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 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. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/115/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/115/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/115/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/115/console This message is automatically generated.
          Hide
          Suresh Srinivas added a comment -

          +1 for the patch.

          Show
          Suresh Srinivas added a comment - +1 for the patch.
          Hide
          Suresh Srinivas added a comment -

          Committed the patch to trunk. Thanks Sanjay.

          Show
          Suresh Srinivas added a comment - Committed the patch to trunk. Thanks Sanjay.

            People

            • Assignee:
              Sanjay Radia
              Reporter:
              Sanjay Radia
            • Votes:
              0 Vote for this issue
              Watchers:
              21 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development