Hadoop Common
  1. Hadoop Common
  2. HADOOP-10557

FsShell -cp -pa option for preserving extended ACLs

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.0
    • Fix Version/s: 2.5.0
    • Component/s: fs
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      This issue tracks enhancing FsShell cp to add a new command-line option (-pa) for preserving extended ACLs.

      1. HADOOP-10557.5.patch
        18 kB
        Akira AJISAKA
      2. HADOOP-10557.4.patch
        14 kB
        Akira AJISAKA
      3. HADOOP-10557.3.patch
        8 kB
        Akira AJISAKA
      4. HADOOP-10557.2.patch
        7 kB
        Akira AJISAKA
      5. HADOOP-10557.patch
        7 kB
        Akira AJISAKA

        Issue Links

          Activity

          Akira AJISAKA created issue -
          Akira AJISAKA made changes -
          Field Original Value New Value
          Link This issue relates to MAPREDUCE-5809 [ MAPREDUCE-5809 ]
          Akira AJISAKA made changes -
          Link This issue relates to HADOOP-9705 [ HADOOP-9705 ]
          Hide
          Akira AJISAKA added a comment -

          I perfer to preserve extended ACLs by -p option rather than creating a new option because cp -p preserves extended ACLs in unix.

          Show
          Akira AJISAKA added a comment - I perfer to preserve extended ACLs by -p option rather than creating a new option because cp -p preserves extended ACLs in unix.
          Akira AJISAKA made changes -
          Assignee Akira AJISAKA [ ajisakaa ]
          Hide
          Akira AJISAKA added a comment -

          Attaching a patch.

          Show
          Akira AJISAKA added a comment - Attaching a patch.
          Akira AJISAKA made changes -
          Attachment HADOOP-10557.patch [ 12646261 ]
          Akira AJISAKA made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Affects Version/s 2.4.0 [ 12326144 ]
          Hide
          Gera Shegalov added a comment -

          That's a good idea, Akira. Some comments:

          @@ -48,7 +55,8 @@
             private boolean preserve = false;
             private boolean verifyChecksum = true;
             private boolean writeChecksum = true;
          -  
          +  private Map<FileSystem, Boolean> supportAclsCache = Maps.newHashMap();
          +
          

          supportAclsCache should be declared as a final instance variable. private final Map...

          +  protected boolean supportAcl(FileSystem fs) {
          +    if (supportAclsCache.containsKey(fs)) {
          +      return supportAclsCache.get(fs);
          +    }
          

          containsKey and get are 2 lookups in the hash map. Do it only once:

          final Boolean res = supportAclsCache.get(fs);
          if (res != null) {
            return res;
          }
          



          +    try {
          +      fs.getAclStatus(new Path(Path.SEPARATOR));
          +    } catch (Exception e) {
          +      supportAclsCache.put(fs, false);
          +      return false;
          +    }
          

          Catch should be only for UnsupportedOperationException, it would probably be useful to log it as well. All other exceptions should be thrown unchanged.

          Show
          Gera Shegalov added a comment - That's a good idea, Akira. Some comments: @@ -48,7 +55,8 @@ private boolean preserve = false ; private boolean verifyChecksum = true ; private boolean writeChecksum = true ; - + private Map<FileSystem, Boolean > supportAclsCache = Maps.newHashMap(); + supportAclsCache should be declared as a final instance variable. private final Map... + protected boolean supportAcl(FileSystem fs) { + if (supportAclsCache.containsKey(fs)) { + return supportAclsCache.get(fs); + } containsKey and get are 2 lookups in the hash map. Do it only once: final Boolean res = supportAclsCache.get(fs); if (res != null ) { return res; } + try { + fs.getAclStatus( new Path(Path.SEPARATOR)); + } catch (Exception e) { + supportAclsCache.put(fs, false ); + return false ; + } Catch should be only for UnsupportedOperationException , it would probably be useful to log it as well. All other exceptions should be thrown unchanged.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3971//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3971//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/12646261/HADOOP-10557.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3971//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3971//console This message is automatically generated.
          Hide
          Akira AJISAKA added a comment -

          Thanks Gera Shegalov for the comment! Updated the patch.

          Show
          Akira AJISAKA added a comment - Thanks Gera Shegalov for the comment! Updated the patch.
          Akira AJISAKA made changes -
          Attachment HADOOP-10557.2.patch [ 12646840 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3972//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3972//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/12646840/HADOOP-10557.2.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3972//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3972//console This message is automatically generated.
          Hide
          Gera Shegalov added a comment -

          The only point I would still raise is that the patch should log one time a warning when the extended ACL's supported by the src.fs have to be dropped because dst.fs cannot handle them.

          Show
          Gera Shegalov added a comment - The only point I would still raise is that the patch should log one time a warning when the extended ACL's supported by the src.fs have to be dropped because dst.fs cannot handle them.
          Gera Shegalov made changes -
          Link This issue is related to HADOOP-10561 [ HADOOP-10561 ]
          Hide
          Akira AJISAKA added a comment -

          Thanks Gera Shegalov for the comment. Updated the patch to print a warning as follows when src has some extended ACLs but dst doesn't support it.

          # hdfs dfs -cp -p testfile file:///root/
          14/06/10 08:28:12 WARN shell.CommandWithDestination: Extended ACLs user:ajisakaa:rw-,group::r-- in hdfs://localhost:9000/user/root/testfile are not preserved because the target FileSystem doesn't support ACLs.
          
          Show
          Akira AJISAKA added a comment - Thanks Gera Shegalov for the comment. Updated the patch to print a warning as follows when src has some extended ACLs but dst doesn't support it. # hdfs dfs -cp -p testfile file: ///root/ 14/06/10 08:28:12 WARN shell.CommandWithDestination: Extended ACLs user:ajisakaa:rw-,group::r-- in hdfs: //localhost:9000/user/root/testfile are not preserved because the target FileSystem doesn't support ACLs.
          Akira AJISAKA made changes -
          Attachment HADOOP-10557.3.patch [ 12649500 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4037//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4037//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/12649500/HADOOP-10557.3.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4037//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4037//console This message is automatically generated.
          Hide
          Chris Nauroth added a comment -

          Akira AJISAKA, thank you for working on this, and Gera Shegalov, thank you for code reviewing. May I take a look before anything gets committed? I expect I'll have time no later than Thursday, 6/12.

          Show
          Chris Nauroth added a comment - Akira AJISAKA , thank you for working on this, and Gera Shegalov , thank you for code reviewing. May I take a look before anything gets committed? I expect I'll have time no later than Thursday, 6/12.
          Hide
          Chris Nauroth added a comment -

          Akira AJISAKA, on Unix, it's true that cp -p attempts to preserve the ACL, but it's possible for this to fail if the destination is on a different file system that doesn't support ACLs. When that happens, cp doesn't just log a warning. It fails with a non-zero exit code. See below for a transcript demonstrating this on Ubuntu. My system is set up so that /home/cnauroth is on a file system with ACL support, but /mnt/share is on a file system without ACL support.

          I think failure is desirable in this case. A user might be surprised that a sensitive file was opened for access after successfully copying it, and log messages don't always give sufficient visibility into the problem.

          [root@ubuntu:pts/0] ~                                                                                               
          > touch /home/cnauroth/fileWithAcl 
          
          [root@ubuntu:pts/0] ~                                                                                               
          > setfacl -m user:cnauroth:rwx /home/cnauroth/fileWithAcl 
          
          [root@ubuntu:pts/0] ~                                                                                               
          > cp /home/cnauroth/fileWithAcl /mnt/share/cnauroth/fileWithAclNoPreserve
          
          [root@ubuntu:pts/0] ~                                                                                               
          > cp -p /home/cnauroth/fileWithAcl /mnt/share/cnauroth/fileWithAclPreserve
          cp: preserving permissions for `/mnt/share/cnauroth/fileWithAclPreserve': Operation not supported
          
          [root@ubuntu:pts/0] ~                                                                                               
          > echo $?
          1
          
          Show
          Chris Nauroth added a comment - Akira AJISAKA , on Unix, it's true that cp -p attempts to preserve the ACL, but it's possible for this to fail if the destination is on a different file system that doesn't support ACLs. When that happens, cp doesn't just log a warning. It fails with a non-zero exit code. See below for a transcript demonstrating this on Ubuntu. My system is set up so that /home/cnauroth is on a file system with ACL support, but /mnt/share is on a file system without ACL support. I think failure is desirable in this case. A user might be surprised that a sensitive file was opened for access after successfully copying it, and log messages don't always give sufficient visibility into the problem. [root@ubuntu:pts/0] ~ > touch /home/cnauroth/fileWithAcl [root@ubuntu:pts/0] ~ > setfacl -m user:cnauroth:rwx /home/cnauroth/fileWithAcl [root@ubuntu:pts/0] ~ > cp /home/cnauroth/fileWithAcl /mnt/share/cnauroth/fileWithAclNoPreserve [root@ubuntu:pts/0] ~ > cp -p /home/cnauroth/fileWithAcl /mnt/share/cnauroth/fileWithAclPreserve cp: preserving permissions for `/mnt/share/cnauroth/fileWithAclPreserve': Operation not supported [root@ubuntu:pts/0] ~ > echo $? 1
          Hide
          Chris Nauroth added a comment -

          Thinking about this a little more, I'm in favor of adding a new command line option instead of building preservation of ACLs into the existing -p.

          As stated in my last comment, I think the failure behavior is important, because it prevents accidentally copying without preserving important authorization rules. For a user copying files across different HDFS clusters, this would unfortunately create a situation where a file on the source cluster could not be copied to the destination cluster while using -p. This has the potential to break existing processes and scripts if a user suddenly sets an ACL in the source cluster. When we consider the possibility of a large recursive cp -p, the impact of the problem becomes even greater. It could fail half-way through a large recursive copy and force a lot of manual recovery work for the operator.

          Another consideration is the additional RPC load. This might be unexpected in very large clusters. Again, the recursive use case magnifies this problem.

          I recommend that we introduce a new -pa option. This way, the new behavior is opt-in, and existing processes won't encounter the problems described above. If there is a problem, then the operator can decide if it makes sense to drop the -pa argument and rerun the command. They'll still have the capability to copy and preserve all other attributes. I realize this deviates from some of the behavior on Unix, but it's a trade-off.

          You may want to wait for HADOOP-10561 to get committed before making any further changes. That patch does something similar for xattrs, and I expect you'll get the opportunity to reuse some code.

          Show
          Chris Nauroth added a comment - Thinking about this a little more, I'm in favor of adding a new command line option instead of building preservation of ACLs into the existing -p . As stated in my last comment, I think the failure behavior is important, because it prevents accidentally copying without preserving important authorization rules. For a user copying files across different HDFS clusters, this would unfortunately create a situation where a file on the source cluster could not be copied to the destination cluster while using -p . This has the potential to break existing processes and scripts if a user suddenly sets an ACL in the source cluster. When we consider the possibility of a large recursive cp -p , the impact of the problem becomes even greater. It could fail half-way through a large recursive copy and force a lot of manual recovery work for the operator. Another consideration is the additional RPC load. This might be unexpected in very large clusters. Again, the recursive use case magnifies this problem. I recommend that we introduce a new -pa option. This way, the new behavior is opt-in, and existing processes won't encounter the problems described above. If there is a problem, then the operator can decide if it makes sense to drop the -pa argument and rerun the command. They'll still have the capability to copy and preserve all other attributes. I realize this deviates from some of the behavior on Unix, but it's a trade-off. You may want to wait for HADOOP-10561 to get committed before making any further changes. That patch does something similar for xattrs, and I expect you'll get the opportunity to reuse some code.
          Chris Nauroth made changes -
          Link This issue requires HADOOP-10561 [ HADOOP-10561 ]
          Hide
          Akira AJISAKA added a comment -

          Thanks Chris Nauroth for the comments! I wait for HADOOP-10561 to get committed.

          Show
          Akira AJISAKA added a comment - Thanks Chris Nauroth for the comments! I wait for HADOOP-10561 to get committed.
          Akira AJISAKA made changes -
          Description This issue tracks enhancing FsShell cp to
          * preserve extended ACLs by -p option
          or
          * add a new command-line option for preserving extended ACLs.
          This issue tracks enhancing FsShell cp to add a new command-line option (-pa) for preserving extended ACLs.
          Hide
          Akira AJISAKA added a comment -

          Updated the patch to add a new option (-pa).

          Show
          Akira AJISAKA added a comment - Updated the patch to add a new option (-pa).
          Akira AJISAKA made changes -
          Attachment HADOOP-10557.4.patch [ 12650207 ]
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4061//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4061//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/12650207/HADOOP-10557.4.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4061//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4061//console This message is automatically generated.
          Hide
          Chris Nauroth added a comment -

          Nice work, Akira! I really appreciate you making the suggested changes. Here are a few comments on v4:

          1. Minor nitpick: let's rename FileAttribute#ACLS to FileAttribute#ACL (singular). This will make it consistent with similar code in distcp. The singular is more correct too. A file has only one access control list, though it may contain multiple entries.
          2. CommandWithDestination: We can make an optimization by checking src.stat.getPermission().getAclBit() before calling src.fs.getAclStatus. This will save some RPC load for the NameNode and reduce latency for the client. See DistCpUtils#toCopyListingFileStatus for an example of similar logic checking the ACL bit.
          3. Speaking of the ACL bit, let's add assertions on the ACL bit in your new tests. Every time you assert the ACL entries are empty, let's also assert that the ACL bit is false. Every time the ACL entries are non-empty, assert that the ACL bit is true.
          4. Nice catch on what looks like an old typo in the test checking target1 when it should be checking target3!
          5. Can you please also write a test that includes a file with both ACL and sticky bit? setAcl cannot set the sticky bit. Only setPermission can do it. I think your logic for handling this is already correct, but let's add a test to make sure and guard against regressions in the future.
          Show
          Chris Nauroth added a comment - Nice work, Akira! I really appreciate you making the suggested changes. Here are a few comments on v4: Minor nitpick: let's rename FileAttribute#ACLS to FileAttribute#ACL (singular). This will make it consistent with similar code in distcp. The singular is more correct too. A file has only one access control list, though it may contain multiple entries. CommandWithDestination : We can make an optimization by checking src.stat.getPermission().getAclBit() before calling src.fs.getAclStatus . This will save some RPC load for the NameNode and reduce latency for the client. See DistCpUtils#toCopyListingFileStatus for an example of similar logic checking the ACL bit. Speaking of the ACL bit, let's add assertions on the ACL bit in your new tests. Every time you assert the ACL entries are empty, let's also assert that the ACL bit is false. Every time the ACL entries are non-empty, assert that the ACL bit is true. Nice catch on what looks like an old typo in the test checking target1 when it should be checking target3 ! Can you please also write a test that includes a file with both ACL and sticky bit? setAcl cannot set the sticky bit. Only setPermission can do it. I think your logic for handling this is already correct, but let's add a test to make sure and guard against regressions in the future.
          Hide
          Akira AJISAKA added a comment -

          Thanks Chris Nauroth for the comment! Updated the patch.

          Show
          Akira AJISAKA added a comment - Thanks Chris Nauroth for the comment! Updated the patch.
          Akira AJISAKA made changes -
          Attachment HADOOP-10557.5.patch [ 12650698 ]
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4079//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4079//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/12650698/HADOOP-10557.5.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4079//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4079//console This message is automatically generated.
          Chris Nauroth made changes -
          Issue Type Bug [ 1 ] Improvement [ 4 ]
          Chris Nauroth made changes -
          Summary FsShell -cp -p does not preserve extended ACLs FsShell -cp -pa option for preserving extended ACLs
          Hide
          Chris Nauroth added a comment -

          +1 for the patch. Thanks for incorporating the feedback. In addition to the automated tests here, I started a 2.3.0 cluster and ran a trunk client build with this change. I verified that there are no compatibility problems, and I verified that the command fails as expected when doing a cp -pa across different file systems and the target file system doesn't support ACLs.

          Akira AJISAKA, while testing I noticed that ACLs were not preserved on directories. Then, I found HADOOP-9705 reporting the bug for all attributes. I'll resume conversation about that on the other jira.

          I'll commit this patch now.

          Show
          Chris Nauroth added a comment - +1 for the patch. Thanks for incorporating the feedback. In addition to the automated tests here, I started a 2.3.0 cluster and ran a trunk client build with this change. I verified that there are no compatibility problems, and I verified that the command fails as expected when doing a cp -pa across different file systems and the target file system doesn't support ACLs. Akira AJISAKA , while testing I noticed that ACLs were not preserved on directories. Then, I found HADOOP-9705 reporting the bug for all attributes. I'll resume conversation about that on the other jira. I'll commit this patch now.
          Chris Nauroth made changes -
          Hadoop Flags Reviewed [ 10343 ]
          Component/s fs [ 12310689 ]
          Hide
          Chris Nauroth added a comment -

          I have committed the patch to trunk and branch-2. Akira, thank you for contributing the patch. Gera, thank you for providing code review feedback.

          Show
          Chris Nauroth added a comment - I have committed the patch to trunk and branch-2. Akira, thank you for contributing the patch. Gera, thank you for providing code review feedback.
          Chris Nauroth made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s 3.0.0 [ 12320357 ]
          Fix Version/s 2.5.0 [ 12326263 ]
          Resolution Fixed [ 1 ]
          Hide
          Akira AJISAKA added a comment -

          Thanks Chris Nauroth for testing!

          Show
          Akira AJISAKA added a comment - Thanks Chris Nauroth for testing!
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #5718 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5718/)
          HADOOP-10557. FsShell -cp -pa option for preserving extended ACLs. Contributed by Akira Ajisaka. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1603222)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/site/apt/FileSystemShell.apt.vm
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #5718 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5718/ ) HADOOP-10557 . FsShell -cp -pa option for preserving extended ACLs. Contributed by Akira Ajisaka. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1603222 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/site/apt/FileSystemShell.apt.vm /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java
          Chris Nauroth made changes -
          Link This issue is related to HADOOP-9705 [ HADOOP-9705 ]
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #587 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/587/)
          HADOOP-10557. FsShell -cp -pa option for preserving extended ACLs. Contributed by Akira Ajisaka. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1603222)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/site/apt/FileSystemShell.apt.vm
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #587 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/587/ ) HADOOP-10557 . FsShell -cp -pa option for preserving extended ACLs. Contributed by Akira Ajisaka. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1603222 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/site/apt/FileSystemShell.apt.vm /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #1778 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1778/)
          HADOOP-10557. FsShell -cp -pa option for preserving extended ACLs. Contributed by Akira Ajisaka. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1603222)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/site/apt/FileSystemShell.apt.vm
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1778 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1778/ ) HADOOP-10557 . FsShell -cp -pa option for preserving extended ACLs. Contributed by Akira Ajisaka. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1603222 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/site/apt/FileSystemShell.apt.vm /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1805 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1805/)
          HADOOP-10557. FsShell -cp -pa option for preserving extended ACLs. Contributed by Akira Ajisaka. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1603222)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/site/apt/FileSystemShell.apt.vm
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1805 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1805/ ) HADOOP-10557 . FsShell -cp -pa option for preserving extended ACLs. Contributed by Akira Ajisaka. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1603222 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CommandWithDestination.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/site/apt/FileSystemShell.apt.vm /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/resources/testConf.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSShell.java
          Karthik Kambatla (Inactive) made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Allen Wittenauer made changes -
          Fix Version/s 3.0.0 [ 12320357 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          21d 1h 44m 1 Akira AJISAKA 22/May/14 10:38
          Patch Available Patch Available Resolved Resolved
          26d 7h 33m 1 Chris Nauroth 17/Jun/14 18:12
          Resolved Resolved Closed Closed
          58d 12h 27m 1 Karthik Kambatla (Inactive) 15/Aug/14 06:39

            People

            • Assignee:
              Akira AJISAKA
              Reporter:
              Akira AJISAKA
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development