Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-5908

Change AclFeature to capture list of ACL entries in an ImmutableList.

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.0
    • Fix Version/s: 3.0.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Target Version/s:

      Description

      After an AclFeature has been attached to an inode, its ACL entries must be immutable. We can enforce this in the type system more strongly by storing the entries in an ImmutableList and changing the getter to return that type instead of raw List.

      1. HDFS-5908.1.patch
        3 kB
        Chris Nauroth

        Issue Links

          Activity

          Chris Nauroth created issue -
          Chris Nauroth made changes -
          Field Original Value New Value
          Link This issue relates to HDFS-4685 [ HDFS-4685 ]
          Chris Nauroth made changes -
          Assignee Chris Nauroth [ cnauroth ]
          Chris Nauroth made changes -
          Priority Major [ 3 ] Minor [ 4 ]
          Hide
          Chris Nauroth added a comment -

          I'm attaching a patch that switches to using ImmutableList in AclFeature.

          Show
          Chris Nauroth added a comment - I'm attaching a patch that switches to using ImmutableList in AclFeature .
          Chris Nauroth made changes -
          Attachment HDFS-5908.1.patch [ 12631115 ]
          Chris Nauroth made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Chris Nauroth made changes -
          Status In Progress [ 3 ] Patch Available [ 10002 ]
          Chris Nauroth made changes -
          Affects Version/s 3.0.0 [ 12320356 ]
          Affects Version/s HDFS ACLs (HDFS-4685) [ 12325671 ]
          Target Version/s HDFS ACLs (HDFS-4685) [ 12325671 ] 3.0.0 [ 12320356 ]
          Hide
          Haohui Mai added a comment -

          Looks good to me. +1 pending Jenkins.

          I wonder, do you have any plans to make similar changes on the methods in AclTransformation? I think it can be addressed in a separate jira though.

          Show
          Haohui Mai added a comment - Looks good to me. +1 pending Jenkins. I wonder, do you have any plans to make similar changes on the methods in AclTransformation ? I think it can be addressed in a separate jira though.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12631115/HDFS-5908.1.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-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6240//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6240//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/12631115/HDFS-5908.1.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-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6240//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6240//console This message is automatically generated.
          Hide
          Chris Nauroth added a comment -

          Thank you for the review, Haohui. I'm going to commit this.

          I wonder, do you have any plans to make similar changes on the methods in AclTransformation?

          I think there is less reason to use ImmutableList inside AclTransformation. For the input ACL spec, the first thing we need to do is sort the entries so that the rest of the algorithms can assume the ordering. Right now, we do that as an in-place sort on the list parsed from the protocol, so it's convenient for that to remain mutable. For the output calculated ACL, it's always the full logical ACL. For the physical storage, we always end up unpacking a few of those entries for mapping onto the permission bits, and the final list stored in the AclFeature is a different, smaller list. I think the change is really important inside AclStorage and AclFeature though to guarantee that there is no accidental mutation of an ACL associated to an existing inode.

          BTW, I just learned that the Guava immutable collections have a smaller memory footprint than the typical JDK data structures:

          https://code.google.com/p/memory-measurer/wiki/ElementCostInDataStructures

          That's a nice side benefit of this patch, and it's worth considering for future changes in areas that consume a lot of memory.

          Show
          Chris Nauroth added a comment - Thank you for the review, Haohui. I'm going to commit this. I wonder, do you have any plans to make similar changes on the methods in AclTransformation ? I think there is less reason to use ImmutableList inside AclTransformation . For the input ACL spec, the first thing we need to do is sort the entries so that the rest of the algorithms can assume the ordering. Right now, we do that as an in-place sort on the list parsed from the protocol, so it's convenient for that to remain mutable. For the output calculated ACL, it's always the full logical ACL. For the physical storage, we always end up unpacking a few of those entries for mapping onto the permission bits, and the final list stored in the AclFeature is a different, smaller list. I think the change is really important inside AclStorage and AclFeature though to guarantee that there is no accidental mutation of an ACL associated to an existing inode. BTW, I just learned that the Guava immutable collections have a smaller memory footprint than the typical JDK data structures: https://code.google.com/p/memory-measurer/wiki/ElementCostInDataStructures That's a nice side benefit of this patch, and it's worth considering for future changes in areas that consume a lot of memory.
          Hide
          Chris Nauroth added a comment -

          I committed this to trunk. Thanks again for the code review, Haohui.

          Show
          Chris Nauroth added a comment - I committed this to trunk. Thanks again for the code review, Haohui.
          Chris Nauroth made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags Reviewed [ 10343 ]
          Fix Version/s 3.0.0 [ 12320356 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #5227 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5227/)
          HDFS-5908. Change AclFeature to capture list of ACL entries in an ImmutableList. Contributed by Chris Nauroth. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1572142)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclFeature.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclStorage.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #5227 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5227/ ) HDFS-5908 . Change AclFeature to capture list of ACL entries in an ImmutableList. Contributed by Chris Nauroth. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1572142 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclFeature.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclStorage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #494 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/494/)
          HDFS-5908. Change AclFeature to capture list of ACL entries in an ImmutableList. Contributed by Chris Nauroth. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1572142)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclFeature.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclStorage.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #494 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/494/ ) HDFS-5908 . Change AclFeature to capture list of ACL entries in an ImmutableList. Contributed by Chris Nauroth. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1572142 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclFeature.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclStorage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Hdfs-trunk #1686 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1686/)
          HDFS-5908. Change AclFeature to capture list of ACL entries in an ImmutableList. Contributed by Chris Nauroth. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1572142)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclFeature.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclStorage.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk #1686 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1686/ ) HDFS-5908 . Change AclFeature to capture list of ACL entries in an ImmutableList. Contributed by Chris Nauroth. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1572142 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclFeature.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclStorage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1711 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1711/)
          HDFS-5908. Change AclFeature to capture list of ACL entries in an ImmutableList. Contributed by Chris Nauroth. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1572142)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclFeature.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclStorage.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1711 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1711/ ) HDFS-5908 . Change AclFeature to capture list of ACL entries in an ImmutableList. Contributed by Chris Nauroth. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1572142 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclFeature.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/AclStorage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSAclBaseTest.java

            People

            • Assignee:
              Chris Nauroth
              Reporter:
              Chris Nauroth
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development