Hadoop Common
  1. Hadoop Common
  2. HADOOP-6869

Functionality to create file or folder on a remote daemon side

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.21.0
    • Fix Version/s: 0.21.0
    • Component/s: test
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Tags:
      herriot

      Description

      Functionality for creating either files or folders in task attempt folder while job is running. The functionality covers the following methods.

      1. public void DaemonProtocol.createFile(String path, String fileName, boolean local) throws IOException;
      It uses to create a file with full permissions.

      2. public void DaemonProtocol.createFile(String path, String fileName, FsPermission permission, boolean local) throws IOException;
      It uses to create a file with given permissions.

      3. public void DaemonProtocol.createFolder(String path, String folderName, boolean local) throws IOException;
      It uses to create a file with full permissions.

      4. public void DaemonProtocol.createFolder(String path, String folderName, FsPermission permission, boolean local) throws IOException;
      It uses to create a folder with given permissions.

      1. 6869-ydist-security.patch
        8 kB
        Vinay Kumar Thota
      2. 6869-ydist-security.patch
        10 kB
        Vinay Kumar Thota
      3. HADOOP-6869.patch
        8 kB
        Vinay Kumar Thota
      4. HADOOP-6869.patch
        9 kB
        Vinay Kumar Thota

        Issue Links

          Activity

          Hide
          Vinay Kumar Thota added a comment -

          Initial patch for yahoo dist security branch.

          Show
          Vinay Kumar Thota added a comment - Initial patch for yahoo dist security branch.
          Hide
          Vinay Kumar Thota added a comment -

          Patch for trunk.

          Show
          Vinay Kumar Thota added a comment - Patch for trunk.
          Hide
          Balaji Rajagopalan added a comment -

          +1

          Show
          Balaji Rajagopalan added a comment - +1
          Hide
          Konstantin Boudnik added a comment -
          • Looks good with one nit. I'd suggest to have methods with fewer arguments simply call ones with more args. E.g. in this case
            public void createFile(String path, String fileName, boolean local) throws IOException {
              createFile(path, fileName, null, local);
            }
            

            instead of duplicating their exact implementation. You have done similar in DaemonProtocolAspect already.

          • Also, in DaemonProtocolAspect you already have a ref to the filesystem so you can just call its createFile methods instead of implementing your own logic
            +  public void DaemonProtocol.createFile(String path, String fileName, 
            +     FsPermission permission, boolean local) throws IOException {
            +    Path p = new Path(path); 
            +    FileSystem fs = getFS(p, local);
            

            just call fs.createNewFile() to have new file created for you.

          • also you are writing some content into this file which seems to be an inadvertent side-effect. Doesn't look like a good idea.
          Show
          Konstantin Boudnik added a comment - Looks good with one nit. I'd suggest to have methods with fewer arguments simply call ones with more args. E.g. in this case public void createFile(String path, String fileName, boolean local) throws IOException { createFile(path, fileName, null, local); } instead of duplicating their exact implementation. You have done similar in DaemonProtocolAspect already. Also, in DaemonProtocolAspect you already have a ref to the filesystem so you can just call its createFile methods instead of implementing your own logic + public void DaemonProtocol.createFile(String path, String fileName, + FsPermission permission, boolean local) throws IOException { + Path p = new Path(path); + FileSystem fs = getFS(p, local); just call fs.createNewFile() to have new file created for you. also you are writing some content into this file which seems to be an inadvertent side-effect. Doesn't look like a good idea.
          Hide
          Konstantin Boudnik added a comment -

          In continuation of the comment above: since there's getFS() method which exposes filesystem API from a particular daemon then creating additional 4 wrappers around the standard API doesn't make much sense to me. These wrappers are too obvious and then will simply crowd Herriot API without adding much value.

          Show
          Konstantin Boudnik added a comment - In continuation of the comment above: since there's getFS() method which exposes filesystem API from a particular daemon then creating additional 4 wrappers around the standard API doesn't make much sense to me. These wrappers are too obvious and then will simply crowd Herriot API without adding much value.
          Hide
          Vinay Kumar Thota added a comment -

          Hi Cos, I do agree with you. However my requirement is creating the user define files and folders in task attempt id folder with different kind of permissions while job is running. Later while cleaning up, i just wanted make sure whether all the user defined files and folders are cleaned up or not for that job. So that I have defined the above methods for fulfilling my requirement. As my understand, i don't think so, the FileSystem is providing the no such straight forward methods(createNewFile and createNewFolder) like you mentioned.

          In the four methods, the first method creates the files with full permission.Suppose if user wants to give his own permissions while creating file in that case he can use the second method.Same way for folder creation also.

          also you are writing some content into this file which seems to be an inadvertent side-effect. Doesn't look like a good idea

          I will remove this part in the code.

          Show
          Vinay Kumar Thota added a comment - Hi Cos, I do agree with you. However my requirement is creating the user define files and folders in task attempt id folder with different kind of permissions while job is running. Later while cleaning up, i just wanted make sure whether all the user defined files and folders are cleaned up or not for that job. So that I have defined the above methods for fulfilling my requirement. As my understand, i don't think so, the FileSystem is providing the no such straight forward methods(createNewFile and createNewFolder) like you mentioned. In the four methods, the first method creates the files with full permission.Suppose if user wants to give his own permissions while creating file in that case he can use the second method.Same way for folder creation also. also you are writing some content into this file which seems to be an inadvertent side-effect. Doesn't look like a good idea I will remove this part in the code.
          Hide
          Konstantin Boudnik added a comment -

          my requirement is creating the user define files and folders in task attempt id folder with different kind of permissions while job is running

          This reasoning sounds like an argument to move this functionality up the stack to the MR cluster concrete implementation. But let's suppose they are useful enough and let them be in the Common.

          In the four methods, the first method creates the files with full permission.Suppose if user wants to give his own permissions while creating file in that case he can use the second method.Same way for folder creation also.

          I do understand that. What I have said is that you need a generic enough method which does most of needed functionality (i.e. create a file with specified permissions) and a wrapper around it which creates a file with all permissions. The implementation of the latter is essentially a call to the former with all permissions being passed. This is already done in this very patch for the implementation of createFolder(..) methods. Look at your own code.

          Show
          Konstantin Boudnik added a comment - my requirement is creating the user define files and folders in task attempt id folder with different kind of permissions while job is running This reasoning sounds like an argument to move this functionality up the stack to the MR cluster concrete implementation. But let's suppose they are useful enough and let them be in the Common. In the four methods, the first method creates the files with full permission.Suppose if user wants to give his own permissions while creating file in that case he can use the second method.Same way for folder creation also. I do understand that. What I have said is that you need a generic enough method which does most of needed functionality (i.e. create a file with specified permissions) and a wrapper around it which creates a file with all permissions. The implementation of the latter is essentially a call to the former with all permissions being passed. This is already done in this very patch for the implementation of createFolder(..) methods. Look at your own code.
          Hide
          Vinay Kumar Thota added a comment -

          Cos,
          Please look at my patch once again because I have not duplicated the exact implementation for create file. I have created a wrapper around the actual method with full permission like which i coded for create folder.Might be you have missed to look at that portion in my patch.

          Show
          Vinay Kumar Thota added a comment - Cos, Please look at my patch once again because I have not duplicated the exact implementation for create file. I have created a wrapper around the actual method with full permission like which i coded for create folder.Might be you have missed to look at that portion in my patch.
          Hide
          Vinay Kumar Thota added a comment -

          Addressed cos comments.

          Show
          Vinay Kumar Thota added a comment - Addressed cos comments.
          Hide
          Konstantin Boudnik added a comment -

          I am glad we have understood each other

          +1 patch looks good. Let's verify. Also, please make sure that it works in real cluster setup because we can't verify it in Hudson yet.

          Show
          Konstantin Boudnik added a comment - I am glad we have understood each other +1 patch looks good. Let's verify. Also, please make sure that it works in real cluster setup because we can't verify it in Hudson yet.
          Hide
          Hadoop QA added a comment -

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

          +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 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/640/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/640/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/640/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/640/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/12450562/HADOOP-6869.patch against trunk revision 979485. +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 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/640/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/640/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/640/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/640/console This message is automatically generated.
          Hide
          Vinay Kumar Thota added a comment -

          java doc warnings are not related to this patch.

          Show
          Vinay Kumar Thota added a comment - java doc warnings are not related to this patch.
          Hide
          Konstantin Boudnik added a comment -

          I'll commit it later today if won't hear otherwise.

          Show
          Konstantin Boudnik added a comment - I'll commit it later today if won't hear otherwise.
          Hide
          Konstantin Boudnik added a comment -

          I have just committed this. Thanks Vinay.

          Show
          Konstantin Boudnik added a comment - I have just committed this. Thanks Vinay.

            People

            • Assignee:
              Vinay Kumar Thota
              Reporter:
              Vinay Kumar Thota
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development