HBase
  1. HBase
  2. HBASE-5526

Configurable file and directory based umask

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.0
    • Component/s: regionserver
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files.

      This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files.

      The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing.

      1. java_HBASE-5526-v7.patch
        12 kB
        Jesse Yates
      2. java_HBASE-5526-v5.patch
        14 kB
        Jesse Yates
      3. java_HBASE-5526-v3.patch
        21 kB
        Jesse Yates
      4. java_HBASE-5526-v2.patch
        93 kB
        Jesse Yates
      5. java_HBASE-5526.patch
        77 kB
        Jesse Yates
      There are no Sub-Tasks for this issue.

        Activity

        Hide
        stack added a comment -

        FYI, hbase-default.xml makes it up into the reference guide. You might then just beef up the hbase-default.xml description (like default seems to be 000 so you might suggest values with description of what the resulting perms will look like). Good stuff lads.

        Show
        stack added a comment - FYI, hbase-default.xml makes it up into the reference guide. You might then just beef up the hbase-default.xml description (like default seems to be 000 so you might suggest values with description of what the resulting perms will look like). Good stuff lads.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #132 (See https://builds.apache.org/job/HBase-TRUNK-security/132/)
        HBASE-5526 ^Cnfigurable file and directory based umask (Jesse Yates) (Revision 1298657)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        • /hbase/trunk/src/main/resources/hbase-default.xml
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #132 (See https://builds.apache.org/job/HBase-TRUNK-security/132/ ) HBASE-5526 ^Cnfigurable file and directory based umask (Jesse Yates) (Revision 1298657) Result = FAILURE larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java /hbase/trunk/src/main/resources/hbase-default.xml /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #21 (See https://builds.apache.org/job/HBase-0.94/21/)
        HBASE-5526 Configurable file and directory based umask (Jesse Yates) (Revision 1298658)

        Result = SUCCESS
        larsh :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/HConstants.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        • /hbase/branches/0.94/src/main/resources/hbase-default.xml
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #21 (See https://builds.apache.org/job/HBase-0.94/21/ ) HBASE-5526 Configurable file and directory based umask (Jesse Yates) (Revision 1298658) Result = SUCCESS larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/HConstants.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java /hbase/branches/0.94/src/main/resources/hbase-default.xml /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2674 (See https://builds.apache.org/job/HBase-TRUNK/2674/)
        HBASE-5526 ^Cnfigurable file and directory based umask (Jesse Yates) (Revision 1298657)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        • /hbase/trunk/src/main/resources/hbase-default.xml
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2674 (See https://builds.apache.org/job/HBase-TRUNK/2674/ ) HBASE-5526 ^Cnfigurable file and directory based umask (Jesse Yates) (Revision 1298657) Result = FAILURE larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java /hbase/trunk/src/main/resources/hbase-default.xml /hbase/trunk/src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java
        Hide
        Lars Hofhansl added a comment -

        Committed to 0.94 and trunk

        Show
        Lars Hofhansl added a comment - Committed to 0.94 and trunk
        Hide
        Lars Hofhansl added a comment -

        Can file a child bug for documentation and why this might be useful.

        Show
        Lars Hofhansl added a comment - Can file a child bug for documentation and why this might be useful.
        Hide
        Jesse Yates added a comment -

        Btw, all failed tests passed locally.

        Do we want to add any extra documentation to ref guide or do you think the standard stuff from hbase-defaults.xml is going to suffice?

        Show
        Jesse Yates added a comment - Btw, all failed tests passed locally. Do we want to add any extra documentation to ref guide or do you think the standard stuff from hbase-defaults.xml is going to suffice?
        Hide
        Lars Hofhansl added a comment -

        Ok... last chance to object.

        Show
        Lars Hofhansl added a comment - Ok... last chance to object.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12517616/java_HBASE-5526-v7.patch
        against trunk revision .

        +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 appears to have generated -127 warning messages.

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

        -1 findbugs. The patch appears to introduce 155 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 failed these unit tests:
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.TestZooKeeper

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1138//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1138//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1138//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/12517616/java_HBASE-5526-v7.patch against trunk revision . +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 appears to have generated -127 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 155 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 failed these unit tests: org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.TestZooKeeper Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1138//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1138//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1138//console This message is automatically generated.
        Hide
        Jesse Yates added a comment -

        Rebased against 0.94.

        I believe this also applies against trunk, if we want to commit it there as well.

        Show
        Jesse Yates added a comment - Rebased against 0.94. I believe this also applies against trunk, if we want to commit it there as well.
        Hide
        Lars Hofhansl added a comment -
        patching file src/main/java/org/apache/hadoop/hbase/HConstants.java
        Hunk #1 succeeded at 610 (offset 4 lines).
        patching file src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java
        patching file src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Hunk #1 succeeded at 67 (offset 2 lines).
        Hunk #2 FAILED at 79.
        Hunk #3 FAILED at 89.
        Hunk #4 succeeded at 342 (offset 4 lines).
        Hunk #5 succeeded at 770 (offset 8 lines).
        2 out of 5 hunks FAILED -- saving rejects to file src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java.rej
        patching file src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        patching file src/main/resources/hbase-default.xml
        patching file src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java
        PATCH APPLICATION FAILED
        

        HRegion was just recently changed.

        Show
        Lars Hofhansl added a comment - patching file src/main/java/org/apache/hadoop/hbase/HConstants.java Hunk #1 succeeded at 610 (offset 4 lines). patching file src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java patching file src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java Hunk #1 succeeded at 67 (offset 2 lines). Hunk #2 FAILED at 79. Hunk #3 FAILED at 89. Hunk #4 succeeded at 342 (offset 4 lines). Hunk #5 succeeded at 770 (offset 8 lines). 2 out of 5 hunks FAILED -- saving rejects to file src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java.rej patching file src/main/java/org/apache/hadoop/hbase/util/FSUtils.java patching file src/main/resources/hbase- default .xml patching file src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java PATCH APPLICATION FAILED HRegion was just recently changed.
        Hide
        Jesse Yates added a comment -

        Yup, works for blocking out the hfiles and .regioninfo.

        Odd that it didn't apply. Going to try rebasing and see if that was the problem.

        Show
        Jesse Yates added a comment - Yup, works for blocking out the hfiles and .regioninfo. Odd that it didn't apply. Going to try rebasing and see if that was the problem.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12517610/java_HBASE-5526-v5.patch
        against trunk revision .

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1137//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/12517610/java_HBASE-5526-v5.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1137//console This message is automatically generated.
        Hide
        Lars Hofhansl added a comment -

        +1

        Will commit soon (if tests pass) and nobody objects.
        Jesse, did you validate that this satisfies our requirements for the JSPs?

        Show
        Lars Hofhansl added a comment - +1 Will commit soon (if tests pass) and nobody objects. Jesse, did you validate that this satisfies our requirements for the JSPs?
        Hide
        Jesse Yates added a comment -

        Applying fixes from RB - should be ready to go.

        Show
        Jesse Yates added a comment - Applying fixes from RB - should be ready to go.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-08 18:39:02, Lars Hofhansl wrote:

        > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 770

        > <https://reviews.apache.org/r/4217/diff/3/?file=89166#file89166line770>

        >

        > We don't log in the HFile case above.

        > I guess we log either in both cases or not at all.

        > LOG.debug in both cases seems fine to me.

        Prefer to drop this as we already have the debug comment in FSUtils.create for the file and perms being created. This is just excessive logging.

        I'll post a new version and try to get most of the whitespace issues on the original ticket.

        • Jesse

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4217/#review5738
        -----------------------------------------------------------

        On 2012-03-08 18:31:13, Jesse Yates wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4217/

        -----------------------------------------------------------

        (Updated 2012-03-08 18:31:13)

        Review request for hbase and Lars Hofhansl.

        Summary

        -------

        Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files.

        This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files.

        The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing.

        This addresses bug HBASE-5526.

        https://issues.apache.org/jira/browse/HBASE-5526

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04

        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe

        src/main/resources/hbase-default.xml 9277e0c

        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6

        Diff: https://reviews.apache.org/r/4217/diff

        Testing

        -------

        "mvn clean test -P localTests" passes

        Thanks,

        Jesse

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-08 18:39:02, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 770 > < https://reviews.apache.org/r/4217/diff/3/?file=89166#file89166line770 > > > We don't log in the HFile case above. > I guess we log either in both cases or not at all. > LOG.debug in both cases seems fine to me. Prefer to drop this as we already have the debug comment in FSUtils.create for the file and perms being created. This is just excessive logging. I'll post a new version and try to get most of the whitespace issues on the original ticket. Jesse ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/#review5738 ----------------------------------------------------------- On 2012-03-08 18:31:13, Jesse Yates wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/ ----------------------------------------------------------- (Updated 2012-03-08 18:31:13) Review request for hbase and Lars Hofhansl. Summary ------- Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files. This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files. The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing. This addresses bug HBASE-5526 . https://issues.apache.org/jira/browse/HBASE-5526 Diffs ----- src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04 src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422 src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe src/main/resources/hbase-default.xml 9277e0c src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6 Diff: https://reviews.apache.org/r/4217/diff Testing ------- "mvn clean test -P localTests" passes Thanks, Jesse
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4217/#review5738
        -----------------------------------------------------------

        Ship it!

        One minor comment and some whitespace (can fix upon commit).

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/4217/#comment12489>

        We don't log in the HFile case above.
        I guess we log either in both cases or not at all.
        LOG.debug in both cases seems fine to me.

        • Lars

        On 2012-03-08 18:31:13, Jesse Yates wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4217/

        -----------------------------------------------------------

        (Updated 2012-03-08 18:31:13)

        Review request for hbase and Lars Hofhansl.

        Summary

        -------

        Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files.

        This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files.

        The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing.

        This addresses bug HBASE-5526.

        https://issues.apache.org/jira/browse/HBASE-5526

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04

        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe

        src/main/resources/hbase-default.xml 9277e0c

        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6

        Diff: https://reviews.apache.org/r/4217/diff

        Testing

        -------

        "mvn clean test -P localTests" passes

        Thanks,

        Jesse

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/#review5738 ----------------------------------------------------------- Ship it! One minor comment and some whitespace (can fix upon commit). src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/4217/#comment12489 > We don't log in the HFile case above. I guess we log either in both cases or not at all. LOG.debug in both cases seems fine to me. Lars On 2012-03-08 18:31:13, Jesse Yates wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/ ----------------------------------------------------------- (Updated 2012-03-08 18:31:13) Review request for hbase and Lars Hofhansl. Summary ------- Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files. This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files. The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing. This addresses bug HBASE-5526 . https://issues.apache.org/jira/browse/HBASE-5526 Diffs ----- src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04 src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422 src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe src/main/resources/hbase-default.xml 9277e0c src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6 Diff: https://reviews.apache.org/r/4217/diff Testing ------- "mvn clean test -P localTests" passes Thanks, Jesse
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4217/
        -----------------------------------------------------------

        (Updated 2012-03-08 18:31:13.452976)

        Review request for hbase and Lars Hofhansl.

        Changes
        -------

        New patch addressing Ted and Lars' comments. Thanks guys!

        Summary
        -------

        Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files.

        This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files.

        The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing.

        This addresses bug HBASE-5526.
        https://issues.apache.org/jira/browse/HBASE-5526

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04
        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422
        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe
        src/main/resources/hbase-default.xml 9277e0c
        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6

        Diff: https://reviews.apache.org/r/4217/diff

        Testing
        -------

        "mvn clean test -P localTests" passes

        Thanks,

        Jesse

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/ ----------------------------------------------------------- (Updated 2012-03-08 18:31:13.452976) Review request for hbase and Lars Hofhansl. Changes ------- New patch addressing Ted and Lars' comments. Thanks guys! Summary ------- Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files. This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files. The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing. This addresses bug HBASE-5526 . https://issues.apache.org/jira/browse/HBASE-5526 Diffs (updated) src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04 src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422 src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe src/main/resources/hbase-default.xml 9277e0c src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6 Diff: https://reviews.apache.org/r/4217/diff Testing ------- "mvn clean test -P localTests" passes Thanks, Jesse
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4217/#review5706
        -----------------------------------------------------------

        src/main/resources/hbase-default.xml
        <https://reviews.apache.org/r/4217/#comment12442>

        This config doesn't match the one on line 873.

        • Ted

        On 2012-03-08 02:56:16, Jesse Yates wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4217/

        -----------------------------------------------------------

        (Updated 2012-03-08 02:56:16)

        Review request for hbase and Lars Hofhansl.

        Summary

        -------

        Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files.

        This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files.

        The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing.

        This addresses bug HBASE-5526.

        https://issues.apache.org/jira/browse/HBASE-5526

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04

        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422

        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe

        src/main/resources/hbase-default.xml 9277e0c

        src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42

        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6

        Diff: https://reviews.apache.org/r/4217/diff

        Testing

        -------

        "mvn clean test -P localTests" passes

        Thanks,

        Jesse

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/#review5706 ----------------------------------------------------------- src/main/resources/hbase-default.xml < https://reviews.apache.org/r/4217/#comment12442 > This config doesn't match the one on line 873. Ted On 2012-03-08 02:56:16, Jesse Yates wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/ ----------------------------------------------------------- (Updated 2012-03-08 02:56:16) Review request for hbase and Lars Hofhansl. Summary ------- Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files. This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files. The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing. This addresses bug HBASE-5526 . https://issues.apache.org/jira/browse/HBASE-5526 Diffs ----- src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04 src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422 src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe src/main/resources/hbase-default.xml 9277e0c src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42 src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6 Diff: https://reviews.apache.org/r/4217/diff Testing ------- "mvn clean test -P localTests" passes Thanks, Jesse
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-08 04:53:06, Lars Hofhansl wrote:

        > src/main/java/org/apache/hadoop/hbase/util/FSUtils.java, line 150

        > <https://reviews.apache.org/r/4217/diff/2/?file=89028#file89028line150>

        >

        > Same here, don't need configuration...

        >

        > And doesn't this change the semantics?

        > Before you'd get an error if the file exists, not you get an error only if pass overwrite as false.

        > Could force overwrite to false I think to retain old behavior.

        No it doesn't because the previous was never actually used This actually preserves the semantics of FileSystem, where its assumed that you overwrite by default.

        • Jesse

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4217/#review5702
        -----------------------------------------------------------

        On 2012-03-08 02:56:16, Jesse Yates wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4217/

        -----------------------------------------------------------

        (Updated 2012-03-08 02:56:16)

        Review request for hbase and Lars Hofhansl.

        Summary

        -------

        Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files.

        This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files.

        The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing.

        This addresses bug HBASE-5526.

        https://issues.apache.org/jira/browse/HBASE-5526

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04

        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422

        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe

        src/main/resources/hbase-default.xml 9277e0c

        src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42

        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6

        Diff: https://reviews.apache.org/r/4217/diff

        Testing

        -------

        "mvn clean test -P localTests" passes

        Thanks,

        Jesse

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-08 04:53:06, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/util/FSUtils.java, line 150 > < https://reviews.apache.org/r/4217/diff/2/?file=89028#file89028line150 > > > Same here, don't need configuration... > > And doesn't this change the semantics? > Before you'd get an error if the file exists, not you get an error only if pass overwrite as false. > Could force overwrite to false I think to retain old behavior. No it doesn't because the previous was never actually used This actually preserves the semantics of FileSystem, where its assumed that you overwrite by default. Jesse ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/#review5702 ----------------------------------------------------------- On 2012-03-08 02:56:16, Jesse Yates wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/ ----------------------------------------------------------- (Updated 2012-03-08 02:56:16) Review request for hbase and Lars Hofhansl. Summary ------- Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files. This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files. The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing. This addresses bug HBASE-5526 . https://issues.apache.org/jira/browse/HBASE-5526 Diffs ----- src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04 src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422 src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe src/main/resources/hbase-default.xml 9277e0c src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42 src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6 Diff: https://reviews.apache.org/r/4217/diff Testing ------- "mvn clean test -P localTests" passes Thanks, Jesse
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-08 04:53:06, Lars Hofhansl wrote:

        > src/main/java/org/apache/hadoop/hbase/util/FSUtils.java, line 188

        > <https://reviews.apache.org/r/4217/diff/2/?file=89028#file89028line188>

        >

        > Ugh... Catching NPE? Better to do a null check for conf above.

        I think is going to be a corner case, since if you enable that stuff, you are expected to also be setting the umasking - was trying to avoid having the extra branching. Can pull it if you still think necessary.

        On 2012-03-08 04:53:06, Lars Hofhansl wrote:

        > src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java, line 157

        > <https://reviews.apache.org/r/4217/diff/2/?file=89031#file89031line157>

        >

        > Maybe also have a test that verifies the actual HBase file permissions.

        > I.e. write some data, then check permissions of HFile.

        +1

        On 2012-03-08 04:53:06, Lars Hofhansl wrote:

        > src/main/java/org/apache/hadoop/hbase/util/FSUtils.java, line 152

        > <https://reviews.apache.org/r/4217/diff/2/?file=89028#file89028line152>

        >

        > Why the – leading the comment?

        Cruft from personal debugging. Going switch it over to debug as well.

        On 2012-03-08 04:53:06, Lars Hofhansl wrote:

        > src/main/java/org/apache/hadoop/hbase/util/FSUtils.java, line 127

        > <https://reviews.apache.org/r/4217/diff/2/?file=89028#file89028line127>

        >

        > Since we pass perms in, we do not need configuration, right?

        > Should overwrite be false for existing behavior?

        +1

        On 2012-03-08 04:53:06, Lars Hofhansl wrote:

        > src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java, line 211

        > <https://reviews.apache.org/r/4217/diff/2/?file=89027#file89027line211>

        >

        > I know we went back and forth on this... But there is nothing in .tableinfo that is secret to a viewer, just some table metadata. So we do not really have to go through this huh-hah

        >

        > Also if we wanted to do this and we always have to create an HBaseConfiguration anyway, why not do this down in writeHTD?

        > That way the conf would not need to be passed down.

        For the first part, thought we had decided the other way around (in fact remember you saying today I forgot to get it with v1 . But yeah, the full htd is up in the hbase ui, so kinda pointless

        "That way the conf would not need to be passed down."
        Wanted to make it explicit from whence the conf came. But yeah, I guess that also works and is cleaner wrt to api changes. Again, pointless since dropping .tableinfo.

        On 2012-03-08 04:53:06, Lars Hofhansl wrote:

        > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 346

        > <https://reviews.apache.org/r/4217/diff/2/?file=89026#file89026line346>

        >

        > Leftover?

        cruft from last time.

        On 2012-03-08 04:53:06, Lars Hofhansl wrote:

        > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 772

        > <https://reviews.apache.org/r/4217/diff/2/?file=89026#file89026line772>

        >

        > Should be HREGION_INFO_PERMISSION_KEY, right?

        > Or I guess HREGION_INFO_PERMISSION_KEY is a leftover?

        The latter. The question here is whether we should be referencing that key directly when doing getFilePermissions or if we should just have that method assume we are getting it from that conf key? Did it this way to make it explicit where the info is coming from - its not a bit change later if we wanted to do multiple levels of 'if nothing set here, then this key'.

        • Jesse

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4217/#review5702
        -----------------------------------------------------------

        On 2012-03-08 02:56:16, Jesse Yates wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4217/

        -----------------------------------------------------------

        (Updated 2012-03-08 02:56:16)

        Review request for hbase and Lars Hofhansl.

        Summary

        -------

        Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files.

        This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files.

        The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing.

        This addresses bug HBASE-5526.

        https://issues.apache.org/jira/browse/HBASE-5526

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04

        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422

        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe

        src/main/resources/hbase-default.xml 9277e0c

        src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42

        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6

        Diff: https://reviews.apache.org/r/4217/diff

        Testing

        -------

        "mvn clean test -P localTests" passes

        Thanks,

        Jesse

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-08 04:53:06, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/util/FSUtils.java, line 188 > < https://reviews.apache.org/r/4217/diff/2/?file=89028#file89028line188 > > > Ugh... Catching NPE? Better to do a null check for conf above. I think is going to be a corner case, since if you enable that stuff, you are expected to also be setting the umasking - was trying to avoid having the extra branching. Can pull it if you still think necessary. On 2012-03-08 04:53:06, Lars Hofhansl wrote: > src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java, line 157 > < https://reviews.apache.org/r/4217/diff/2/?file=89031#file89031line157 > > > Maybe also have a test that verifies the actual HBase file permissions. > I.e. write some data, then check permissions of HFile. +1 On 2012-03-08 04:53:06, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/util/FSUtils.java, line 152 > < https://reviews.apache.org/r/4217/diff/2/?file=89028#file89028line152 > > > Why the – leading the comment? Cruft from personal debugging. Going switch it over to debug as well. On 2012-03-08 04:53:06, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/util/FSUtils.java, line 127 > < https://reviews.apache.org/r/4217/diff/2/?file=89028#file89028line127 > > > Since we pass perms in, we do not need configuration, right? > Should overwrite be false for existing behavior? +1 On 2012-03-08 04:53:06, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java, line 211 > < https://reviews.apache.org/r/4217/diff/2/?file=89027#file89027line211 > > > I know we went back and forth on this... But there is nothing in .tableinfo that is secret to a viewer, just some table metadata. So we do not really have to go through this huh-hah > > Also if we wanted to do this and we always have to create an HBaseConfiguration anyway, why not do this down in writeHTD? > That way the conf would not need to be passed down. For the first part, thought we had decided the other way around (in fact remember you saying today I forgot to get it with v1 . But yeah, the full htd is up in the hbase ui, so kinda pointless "That way the conf would not need to be passed down." Wanted to make it explicit from whence the conf came. But yeah, I guess that also works and is cleaner wrt to api changes. Again, pointless since dropping .tableinfo. On 2012-03-08 04:53:06, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 346 > < https://reviews.apache.org/r/4217/diff/2/?file=89026#file89026line346 > > > Leftover? cruft from last time. On 2012-03-08 04:53:06, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 772 > < https://reviews.apache.org/r/4217/diff/2/?file=89026#file89026line772 > > > Should be HREGION_INFO_PERMISSION_KEY, right? > Or I guess HREGION_INFO_PERMISSION_KEY is a leftover? The latter. The question here is whether we should be referencing that key directly when doing getFilePermissions or if we should just have that method assume we are getting it from that conf key? Did it this way to make it explicit where the info is coming from - its not a bit change later if we wanted to do multiple levels of 'if nothing set here, then this key'. Jesse ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/#review5702 ----------------------------------------------------------- On 2012-03-08 02:56:16, Jesse Yates wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/ ----------------------------------------------------------- (Updated 2012-03-08 02:56:16) Review request for hbase and Lars Hofhansl. Summary ------- Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files. This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files. The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing. This addresses bug HBASE-5526 . https://issues.apache.org/jira/browse/HBASE-5526 Diffs ----- src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04 src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422 src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe src/main/resources/hbase-default.xml 9277e0c src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42 src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6 Diff: https://reviews.apache.org/r/4217/diff Testing ------- "mvn clean test -P localTests" passes Thanks, Jesse
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4217/#review5702
        -----------------------------------------------------------

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/4217/#comment12427>

        Leftover?

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/4217/#comment12423>

        Should be HREGION_INFO_PERMISSION_KEY, right?
        Or I guess HREGION_INFO_PERMISSION_KEY is a leftover?

        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
        <https://reviews.apache.org/r/4217/#comment12428>

        I know we went back and forth on this... But there is nothing in .tableinfo that is secret to a viewer, just some table metadata. So we do not really have to go through this huh-hah

        Also if we wanted to do this and we always have to create an HBaseConfiguration anyway, why not do this down in writeHTD?
        That way the conf would not need to be passed down.

        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
        <https://reviews.apache.org/r/4217/#comment12431>

        above you call fs.create(...), why using FSUtils here? Seems we could remove the extra create method from FSUtils, since it is only called from here.

        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
        <https://reviews.apache.org/r/4217/#comment12432>

        Just make the HBaseConfiguration in writeHTD (or as said above don't worry about .tableinfos)

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        <https://reviews.apache.org/r/4217/#comment12425>

        Since we pass perms in, we do not need configuration, right?
        Should overwrite be false for existing behavior?

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        <https://reviews.apache.org/r/4217/#comment12424>

        Same here, don't need configuration...

        And doesn't this change the semantics?
        Before you'd get an error if the file exists, not you get an error only if pass overwrite as false.
        Could force overwrite to false I think to retain old behavior.

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        <https://reviews.apache.org/r/4217/#comment12426>

        Why the – leading the comment?

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        <https://reviews.apache.org/r/4217/#comment12429>

        Ugh... Catching NPE? Better to do a null check for conf above.

        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java
        <https://reviews.apache.org/r/4217/#comment12430>

        Maybe also have a test that verifies the actual HBase file permissions.
        I.e. write some data, then check permissions of HFile.

        • Lars

        On 2012-03-08 02:56:16, Jesse Yates wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4217/

        -----------------------------------------------------------

        (Updated 2012-03-08 02:56:16)

        Review request for hbase and Lars Hofhansl.

        Summary

        -------

        Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files.

        This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files.

        The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing.

        This addresses bug HBASE-5526.

        https://issues.apache.org/jira/browse/HBASE-5526

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04

        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422

        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe

        src/main/resources/hbase-default.xml 9277e0c

        src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42

        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6

        Diff: https://reviews.apache.org/r/4217/diff

        Testing

        -------

        "mvn clean test -P localTests" passes

        Thanks,

        Jesse

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/#review5702 ----------------------------------------------------------- src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/4217/#comment12427 > Leftover? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/4217/#comment12423 > Should be HREGION_INFO_PERMISSION_KEY, right? Or I guess HREGION_INFO_PERMISSION_KEY is a leftover? src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java < https://reviews.apache.org/r/4217/#comment12428 > I know we went back and forth on this... But there is nothing in .tableinfo that is secret to a viewer, just some table metadata. So we do not really have to go through this huh-hah Also if we wanted to do this and we always have to create an HBaseConfiguration anyway, why not do this down in writeHTD? That way the conf would not need to be passed down. src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java < https://reviews.apache.org/r/4217/#comment12431 > above you call fs.create(...), why using FSUtils here? Seems we could remove the extra create method from FSUtils, since it is only called from here. src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java < https://reviews.apache.org/r/4217/#comment12432 > Just make the HBaseConfiguration in writeHTD (or as said above don't worry about .tableinfos) src/main/java/org/apache/hadoop/hbase/util/FSUtils.java < https://reviews.apache.org/r/4217/#comment12425 > Since we pass perms in, we do not need configuration, right? Should overwrite be false for existing behavior? src/main/java/org/apache/hadoop/hbase/util/FSUtils.java < https://reviews.apache.org/r/4217/#comment12424 > Same here, don't need configuration... And doesn't this change the semantics? Before you'd get an error if the file exists, not you get an error only if pass overwrite as false. Could force overwrite to false I think to retain old behavior. src/main/java/org/apache/hadoop/hbase/util/FSUtils.java < https://reviews.apache.org/r/4217/#comment12426 > Why the – leading the comment? src/main/java/org/apache/hadoop/hbase/util/FSUtils.java < https://reviews.apache.org/r/4217/#comment12429 > Ugh... Catching NPE? Better to do a null check for conf above. src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java < https://reviews.apache.org/r/4217/#comment12430 > Maybe also have a test that verifies the actual HBase file permissions. I.e. write some data, then check permissions of HFile. Lars On 2012-03-08 02:56:16, Jesse Yates wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/ ----------------------------------------------------------- (Updated 2012-03-08 02:56:16) Review request for hbase and Lars Hofhansl. Summary ------- Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files. This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files. The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing. This addresses bug HBASE-5526 . https://issues.apache.org/jira/browse/HBASE-5526 Diffs ----- src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04 src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422 src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe src/main/resources/hbase-default.xml 9277e0c src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42 src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6 Diff: https://reviews.apache.org/r/4217/diff Testing ------- "mvn clean test -P localTests" passes Thanks, Jesse
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4217/
        -----------------------------------------------------------

        (Updated 2012-03-08 02:56:16.219372)

        Review request for hbase and Lars Hofhansl.

        Changes
        -------

        Simpler version of the previous patch that just umasks the data files created by hbase (hfiles, .regioninfo, .tableinfo).

        Summary
        -------

        Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files.

        This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files.

        The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing.

        This addresses bug HBASE-5526.
        https://issues.apache.org/jira/browse/HBASE-5526

        Diffs (updated)


        src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04
        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422
        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac
        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe
        src/main/resources/hbase-default.xml 9277e0c
        src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42
        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6

        Diff: https://reviews.apache.org/r/4217/diff

        Testing
        -------

        "mvn clean test -P localTests" passes

        Thanks,

        Jesse

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/ ----------------------------------------------------------- (Updated 2012-03-08 02:56:16.219372) Review request for hbase and Lars Hofhansl. Changes ------- Simpler version of the previous patch that just umasks the data files created by hbase (hfiles, .regioninfo, .tableinfo). Summary ------- Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files. This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files. The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing. This addresses bug HBASE-5526 . https://issues.apache.org/jira/browse/HBASE-5526 Diffs (updated) src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04 src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624 src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422 src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe src/main/resources/hbase-default.xml 9277e0c src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42 src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6 Diff: https://reviews.apache.org/r/4217/diff Testing ------- "mvn clean test -P localTests" passes Thanks, Jesse
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4217/
        -----------------------------------------------------------

        (Updated 2012-03-08 02:55:22.079338)

        Review request for hbase and Lars Hofhansl.

        Changes
        -------

        Updating review description and title

        Summary (updated)
        -------

        Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files.

        This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files.

        The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing.

        This addresses bug HBASE-5526.
        https://issues.apache.org/jira/browse/HBASE-5526

        Diffs


        src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04
        src/main/java/org/apache/hadoop/hbase/io/Reference.java 6360059
        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624
        src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java c726603
        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java 8a7da2e
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422
        src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 3f42efa
        src/main/java/org/apache/hadoop/hbase/regionserver/Store.java d884598
        src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 12ebc0a
        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java b5049b1
        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 5c8fc5e
        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac
        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe
        src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 720841c
        src/main/java/org/apache/hadoop/hbase/util/RegionSplitter.java f6d088f
        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9
        src/main/resources/hbase-default.xml 9277e0c
        src/test/java/org/apache/hadoop/hbase/TestFSTableDescriptorForceCreation.java 6bc7c32
        src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java a5628b9
        src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java 5b3b962
        src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java f1ea701
        src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42
        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6

        Diff: https://reviews.apache.org/r/4217/diff

        Testing
        -------

        "mvn clean test -P localTests" passes

        Thanks,

        Jesse

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/ ----------------------------------------------------------- (Updated 2012-03-08 02:55:22.079338) Review request for hbase and Lars Hofhansl. Changes ------- Updating review description and title Summary (updated) ------- Currently many all the files created by the HBase user are just written using the default file permissions granted by hdfs. However, to ensure only the correct user/group views the files and directories, we need to be able to apply a configurable umask to either directories or files. This ticket covers setting permissions for files written to dfs, as opposed to things like pid and log files. The impetus for this was to allow the web-user to view the directory structure of hbase, but not to actually see any of the actual data hbase is storing. This addresses bug HBASE-5526 . https://issues.apache.org/jira/browse/HBASE-5526 Diffs src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04 src/main/java/org/apache/hadoop/hbase/io/Reference.java 6360059 src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624 src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java c726603 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java 8a7da2e src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422 src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 3f42efa src/main/java/org/apache/hadoop/hbase/regionserver/Store.java d884598 src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 12ebc0a src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java b5049b1 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 5c8fc5e src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 720841c src/main/java/org/apache/hadoop/hbase/util/RegionSplitter.java f6d088f src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9 src/main/resources/hbase-default.xml 9277e0c src/test/java/org/apache/hadoop/hbase/TestFSTableDescriptorForceCreation.java 6bc7c32 src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java a5628b9 src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java 5b3b962 src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java f1ea701 src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42 src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6 Diff: https://reviews.apache.org/r/4217/diff Testing ------- "mvn clean test -P localTests" passes Thanks, Jesse
        Hide
        Jesse Yates added a comment -

        Simpler patch than v2 - just covers up the hfiles, .regioninfo and .tableinfo.

        The last was a bit more complicated than I would like as a TableDescriptor doesn't implicitly have a configuration, but it manages.

        The other oddity is that this patch takes a conf key when getting the permissions - did this to help keep it abstracted out, but it could debatably just drop that param and always look to using the default (and maybe have an overloaded method that takes the first to check and then falls back on hbase.data.umask if that value isn't set). Definitely up for debate here.

        Throwing this up on RB as well.

        Show
        Jesse Yates added a comment - Simpler patch than v2 - just covers up the hfiles, .regioninfo and .tableinfo. The last was a bit more complicated than I would like as a TableDescriptor doesn't implicitly have a configuration, but it manages. The other oddity is that this patch takes a conf key when getting the permissions - did this to help keep it abstracted out, but it could debatably just drop that param and always look to using the default (and maybe have an overloaded method that takes the first to check and then falls back on hbase.data.umask if that value isn't set). Definitely up for debate here. Throwing this up on RB as well.
        Hide
        Lars Hofhansl added a comment -

        +1 on simplified version of first patch.

        Show
        Lars Hofhansl added a comment - +1 on simplified version of first patch.
        Hide
        Jesse Yates added a comment -

        After talking with Lars, this most recent patch seems really heavyweight to accomplish the original intention of not seeing data files in the jsp. As it stands now (with either patch), we will still be able to see a lot of the information in the hbase webui, so full scale blocking is not really gaining a lot.

        Further, we would need to ensure that all future file or directory creation goes through the FSUtils tool - something that is going to quite annoying and difficult to enforce. This also makes it slightly more of a pain later on, if we want to add more specific umasking to the certain files as they will need to pass in their own umask.

        Instead, proposing to simplify the original patch such that we only have 2 configuration keys - enable and data.umask. The former would turn on special umasking and the latter would initially be used when applying a umask to all the 'data' files in hbase. This is nice in that we can later make the latter the default value if we want to apply extra special umasking to a given file (via a new conf key).

        Show
        Jesse Yates added a comment - After talking with Lars, this most recent patch seems really heavyweight to accomplish the original intention of not seeing data files in the jsp. As it stands now (with either patch), we will still be able to see a lot of the information in the hbase webui, so full scale blocking is not really gaining a lot. Further, we would need to ensure that all future file or directory creation goes through the FSUtils tool - something that is going to quite annoying and difficult to enforce. This also makes it slightly more of a pain later on, if we want to add more specific umasking to the certain files as they will need to pass in their own umask. Instead, proposing to simplify the original patch such that we only have 2 configuration keys - enable and data.umask. The former would turn on special umasking and the latter would initially be used when applying a umask to all the 'data' files in hbase. This is nice in that we can later make the latter the default value if we want to apply extra special umasking to a given file (via a new conf key).
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4217/#review5681
        -----------------------------------------------------------

        Looks good.
        Some unnecessary reformating and some whitespace issues.
        Also see minor comments inside.
        Should probably give the motivation for the change (i.e. allow some users to browse the HBase directory hierarchy, but not see the contents of any HFile or HLog files.)

        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java
        <https://reviews.apache.org/r/4217/#comment12385>

        Is this a left-over from an older patch?
        I don't see how FSUTils.create* could know which KEY to use to read the perms from.

        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java
        <https://reviews.apache.org/r/4217/#comment12383>

        Is "conf" different from "c" here? Should it?
        (Maybe I am not grokking this part)

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/4217/#comment12384>

        left-over from an older patch?

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/4217/#comment12387>

        Instead of using the static methods, would it be possible to instantiate a FSUtils object per region (or maybe even region server) and configure that instance with a Configuration?
        Might make it worse... Just checking.

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/4217/#comment12386>

        unnecessary reformat

        src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
        <https://reviews.apache.org/r/4217/#comment12388>

        pigurationaram? Is that an animal?

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        <https://reviews.apache.org/r/4217/#comment12389>

        Unrelated layout changes.

        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java
        <https://reviews.apache.org/r/4217/#comment12390>

        Ugh... I guess there is no way around this.

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        <https://reviews.apache.org/r/4217/#comment12391>

        Just gone?

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        <https://reviews.apache.org/r/4217/#comment12392>

        Unrelated

        • Lars

        On 2012-03-07 06:54:11, Jesse Yates wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4217/

        -----------------------------------------------------------

        (Updated 2012-03-07 06:54:11)

        Review request for hbase and Lars Hofhansl.

        Summary

        -------

        Add support for a configurable umask all files and directories created on the dfs (as opposed to pids or logs).

        This addresses bug HBASE-5526.

        https://issues.apache.org/jira/browse/HBASE-5526

        Diffs

        -----

        src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04

        src/main/java/org/apache/hadoop/hbase/io/Reference.java 6360059

        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624

        src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java c726603

        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java 8a7da2e

        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422

        src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 3f42efa

        src/main/java/org/apache/hadoop/hbase/regionserver/Store.java d884598

        src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 12ebc0a

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java b5049b1

        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 5c8fc5e

        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac

        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe

        src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 720841c

        src/main/java/org/apache/hadoop/hbase/util/RegionSplitter.java f6d088f

        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9

        src/main/resources/hbase-default.xml 9277e0c

        src/test/java/org/apache/hadoop/hbase/TestFSTableDescriptorForceCreation.java 6bc7c32

        src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java a5628b9

        src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java 5b3b962

        src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java f1ea701

        src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42

        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6

        Diff: https://reviews.apache.org/r/4217/diff

        Testing

        -------

        "mvn clean test -P localTests" passes

        Thanks,

        Jesse

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/#review5681 ----------------------------------------------------------- Looks good. Some unnecessary reformating and some whitespace issues. Also see minor comments inside. Should probably give the motivation for the change (i.e. allow some users to browse the HBase directory hierarchy, but not see the contents of any HFile or HLog files.) src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java < https://reviews.apache.org/r/4217/#comment12385 > Is this a left-over from an older patch? I don't see how FSUTils.create* could know which KEY to use to read the perms from. src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java < https://reviews.apache.org/r/4217/#comment12383 > Is "conf" different from "c" here? Should it? (Maybe I am not grokking this part) src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/4217/#comment12384 > left-over from an older patch? src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/4217/#comment12387 > Instead of using the static methods, would it be possible to instantiate a FSUtils object per region (or maybe even region server) and configure that instance with a Configuration? Might make it worse... Just checking. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/4217/#comment12386 > unnecessary reformat src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java < https://reviews.apache.org/r/4217/#comment12388 > pigurationaram? Is that an animal? src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java < https://reviews.apache.org/r/4217/#comment12389 > Unrelated layout changes. src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java < https://reviews.apache.org/r/4217/#comment12390 > Ugh... I guess there is no way around this. src/main/java/org/apache/hadoop/hbase/util/FSUtils.java < https://reviews.apache.org/r/4217/#comment12391 > Just gone? src/main/java/org/apache/hadoop/hbase/util/FSUtils.java < https://reviews.apache.org/r/4217/#comment12392 > Unrelated Lars On 2012-03-07 06:54:11, Jesse Yates wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/ ----------------------------------------------------------- (Updated 2012-03-07 06:54:11) Review request for hbase and Lars Hofhansl. Summary ------- Add support for a configurable umask all files and directories created on the dfs (as opposed to pids or logs). This addresses bug HBASE-5526 . https://issues.apache.org/jira/browse/HBASE-5526 Diffs ----- src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04 src/main/java/org/apache/hadoop/hbase/io/Reference.java 6360059 src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624 src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java c726603 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java 8a7da2e src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422 src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 3f42efa src/main/java/org/apache/hadoop/hbase/regionserver/Store.java d884598 src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 12ebc0a src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java b5049b1 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 5c8fc5e src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 720841c src/main/java/org/apache/hadoop/hbase/util/RegionSplitter.java f6d088f src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9 src/main/resources/hbase-default.xml 9277e0c src/test/java/org/apache/hadoop/hbase/TestFSTableDescriptorForceCreation.java 6bc7c32 src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java a5628b9 src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java 5b3b962 src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java f1ea701 src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42 src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6 Diff: https://reviews.apache.org/r/4217/diff Testing ------- "mvn clean test -P localTests" passes Thanks, Jesse
        Hide
        Lars Hofhansl added a comment -

        Note that since 'hbase.files.permissions.enabled' - should probably be renamed - is false, the default will be to use the filesystem defaults

        Oh I see. That makes sense.

        Show
        Lars Hofhansl added a comment - Note that since 'hbase.files.permissions.enabled' - should probably be renamed - is false, the default will be to use the filesystem defaults Oh I see. That makes sense.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4217/
        -----------------------------------------------------------

        Review request for hbase and Lars Hofhansl.

        Summary
        -------

        Add support for a configurable umask all files and directories created on the dfs (as opposed to pids or logs).

        This addresses bug HBASE-5526.
        https://issues.apache.org/jira/browse/HBASE-5526

        Diffs


        src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04
        src/main/java/org/apache/hadoop/hbase/io/Reference.java 6360059
        src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624
        src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java c726603
        src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java 8a7da2e
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422
        src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 3f42efa
        src/main/java/org/apache/hadoop/hbase/regionserver/Store.java d884598
        src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 12ebc0a
        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java b5049b1
        src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 5c8fc5e
        src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac
        src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe
        src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 720841c
        src/main/java/org/apache/hadoop/hbase/util/RegionSplitter.java f6d088f
        src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9
        src/main/resources/hbase-default.xml 9277e0c
        src/test/java/org/apache/hadoop/hbase/TestFSTableDescriptorForceCreation.java 6bc7c32
        src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java a5628b9
        src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java 5b3b962
        src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java f1ea701
        src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42
        src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6

        Diff: https://reviews.apache.org/r/4217/diff

        Testing
        -------

        "mvn clean test -P localTests" passes

        Thanks,

        Jesse

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4217/ ----------------------------------------------------------- Review request for hbase and Lars Hofhansl. Summary ------- Add support for a configurable umask all files and directories created on the dfs (as opposed to pids or logs). This addresses bug HBASE-5526 . https://issues.apache.org/jira/browse/HBASE-5526 Diffs src/main/java/org/apache/hadoop/hbase/HConstants.java e60ce04 src/main/java/org/apache/hadoop/hbase/io/Reference.java 6360059 src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 9e7e624 src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java c726603 src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java 8a7da2e src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 76ff422 src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 3f42efa src/main/java/org/apache/hadoop/hbase/regionserver/Store.java d884598 src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 12ebc0a src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java b5049b1 src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 5c8fc5e src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java 62cf6ac src/main/java/org/apache/hadoop/hbase/util/FSUtils.java d2d7efe src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 720841c src/main/java/org/apache/hadoop/hbase/util/RegionSplitter.java f6d088f src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9 src/main/resources/hbase-default.xml 9277e0c src/test/java/org/apache/hadoop/hbase/TestFSTableDescriptorForceCreation.java 6bc7c32 src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java a5628b9 src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java 5b3b962 src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java f1ea701 src/test/java/org/apache/hadoop/hbase/util/TestFSTableDescriptors.java 0db4d42 src/test/java/org/apache/hadoop/hbase/util/TestFSUtils.java e2611e6 Diff: https://reviews.apache.org/r/4217/diff Testing ------- "mvn clean test -P localTests" passes Thanks, Jesse
        Hide
        Jesse Yates added a comment -

        Kinda nasty that the file operations now need a Configuration object

        Indeed, but all the sources pretty much have it directly (or pretty close) available. Thinking about adding a method that doesn't need one, but instead just uses HBaseConfiguration.create() as its conf (which is what people are probably going to do implicitly in the future without that overloaded method).

        Would probably not hardcode 022 as the default umask

        This is the default umask that hadoop applies to files, so I figured it would be fine as that. I feel like these conf values should be the defaults, but maybe just wide open? Note that since 'hbase.files.permissions.enabled' - should probably be renamed - is false, the default will be to use the filesystem defaults and ignore the conf values (also stated in the description of those params in the conf).

        Wanna put up RB?

        Done.

        Show
        Jesse Yates added a comment - Kinda nasty that the file operations now need a Configuration object Indeed, but all the sources pretty much have it directly (or pretty close) available. Thinking about adding a method that doesn't need one, but instead just uses HBaseConfiguration.create() as its conf (which is what people are probably going to do implicitly in the future without that overloaded method). Would probably not hardcode 022 as the default umask This is the default umask that hadoop applies to files, so I figured it would be fine as that. I feel like these conf values should be the defaults, but maybe just wide open? Note that since 'hbase.files.permissions.enabled' - should probably be renamed - is false, the default will be to use the filesystem defaults and ignore the conf values (also stated in the description of those params in the conf). Wanna put up RB? Done.
        Hide
        Lars Hofhansl added a comment -

        Wow. Big patch. Kinda nasty that the file operations now need a Configuration object. On the other hand it's nice to have it available there now, let's us pass more config options in the future without rewriting.

        Initial comment. Would probably not hardcode 022 as the default umask (neither in code, nor in hbase-defaults.xml, but rather use FS' default).

        Wanna put up RB?

        Show
        Lars Hofhansl added a comment - Wow. Big patch. Kinda nasty that the file operations now need a Configuration object. On the other hand it's nice to have it available there now, let's us pass more config options in the future without rewriting. Initial comment. Would probably not hardcode 022 as the default umask (neither in code, nor in hbase-defaults.xml, but rather use FS' default). Wanna put up RB?
        Hide
        Jesse Yates added a comment -

        Attaching patch for general umask application for files and directories.

        Essentially, the patch takes all the file and directory creation in src/main and pipes it through new methods in FSUtils, which automatically apply the file permissions if they are turned on, rather than just doing the straigh fs.create() or fs.mkdirs() calls.

        Unfortunately, this is a wider sweeping change than anticipated as there are a lot of places that create files and directories. However, this helps centralize the code in the future, if we want to more/fancier stuff to the files.

        Show
        Jesse Yates added a comment - Attaching patch for general umask application for files and directories. Essentially, the patch takes all the file and directory creation in src/main and pipes it through new methods in FSUtils, which automatically apply the file permissions if they are turned on, rather than just doing the straigh fs.create() or fs.mkdirs() calls. Unfortunately, this is a wider sweeping change than anticipated as there are a lot of places that create files and directories. However, this helps centralize the code in the future, if we want to more/fancier stuff to the files.
        Hide
        Jesse Yates added a comment -

        Side note for other interested parties - just using dfs.umaskmode from hadoop isn't sufficient here as we want to protect the data files, but still see the directory structure. In true posix style, hdfs just applies the umask to all the files and directories, as opposed to differentiating between the two.

        Show
        Jesse Yates added a comment - Side note for other interested parties - just using dfs.umaskmode from hadoop isn't sufficient here as we want to protect the data files, but still see the directory structure. In true posix style, hdfs just applies the umask to all the files and directories, as opposed to differentiating between the two.
        Hide
        Jesse Yates added a comment -

        take the default from the FS (which is the current behavior, right?)

        yup

        Would be nice if we could put entries there just for documentation purposes, but not actually providing a default.

        Exactly what I was thinking, especially as the doc page in the ref guide is generated from hbase-default.xml

        Show
        Jesse Yates added a comment - take the default from the FS (which is the current behavior, right?) yup Would be nice if we could put entries there just for documentation purposes, but not actually providing a default. Exactly what I was thinking, especially as the doc page in the ref guide is generated from hbase-default.xml
        Hide
        Lars Hofhansl added a comment -

        Fair enough
        To your earlier comment, I think the default behavior should be to take the default from the FS (which is the current behavior, right?)

        I think that also means that we do not want to put this in hbase-defaults.xml.
        Meh. (Would be nice if we could put entries there just for documentation purposes, but not actually providing a default).

        Show
        Lars Hofhansl added a comment - Fair enough To your earlier comment, I think the default behavior should be to take the default from the FS (which is the current behavior, right?) I think that also means that we do not want to put this in hbase-defaults.xml. Meh. (Would be nice if we could put entries there just for documentation purposes, but not actually providing a default).
        Hide
        Jesse Yates added a comment -

        In short, if we do allow setting umasks, we should do it for both directories and files - doing it right the first (ok, second) time

        Show
        Jesse Yates added a comment - In short, if we do allow setting umasks, we should do it for both directories and files - doing it right the first (ok, second) time
        Hide
        Jesse Yates added a comment -

        Problem is we make calls to o.a.hadoop.fs.FileSystem.create(...) or FileSystem.mkdirs(Path), which make a call to FsPermissions.getDefault() in those methods.

        The meat of the patch above is to change the call to actually pass in an instance of FsPermissions, rather than taking the default, which seems necessary to change if we want to apply a umask since that method call is just:

          /** Get the default permission. */
          public static FsPermission getDefault() {
            return new FsPermission((short)0777);
          }
        

        which means we would still need to replace a bunch of the calls.

        Show
        Jesse Yates added a comment - Problem is we make calls to o.a.hadoop.fs.FileSystem.create(...) or FileSystem.mkdirs(Path), which make a call to FsPermissions.getDefault() in those methods. The meat of the patch above is to change the call to actually pass in an instance of FsPermissions, rather than taking the default, which seems necessary to change if we want to apply a umask since that method call is just: /** Get the default permission. */ public static FsPermission getDefault() { return new FsPermission(( short )0777); } which means we would still need to replace a bunch of the calls.
        Hide
        Lars Hofhansl added a comment -

        Since the background for this is to be able to browse the directories through the HDFS overview JSPs but to disallow downloading of the actual files, maybe we can just use the FS' default umask for HBase created directories and only override for HBase created files...?

        Show
        Lars Hofhansl added a comment - Since the background for this is to be able to browse the directories through the HDFS overview JSPs but to disallow downloading of the actual files, maybe we can just use the FS' default umask for HBase created directories and only override for HBase created files...?
        Hide
        Jesse Yates added a comment -

        What's with all the changes to hbase-defaults.xml?

        Oops, eclipse auto-formatting; thought I had reverted... Should only be the addition of those last three properties.

        Also, was debating if we even want to include default values for the permissions/umask as the actual 'default' would be taken from the filesystem, and not the conf, so default values are only applied when we enable the permissions stuff, so you should be changing them anyways. Figure wide open (777) permissions as the 'default' was the best, but thought it would be good to get another opinion would be good too.

        Would that not cover all interesting scenarios?

        Yeah, it would. This was just the easiest way to go about covering the identifiable data as there are a bunch of places where we are creating files on the fs and seemed a bit overkill to replace all of them for this use case.

        For creating individual files, its <10 files in src/main for creating files, though a fair number in src/test. However, for creating directories its twice that number and even more for testing. Each of those would need to be wrapped to set the default permissions, doable and starting to think that it's the right way to go about it in the end...hmmm

        Show
        Jesse Yates added a comment - What's with all the changes to hbase-defaults.xml? Oops, eclipse auto-formatting; thought I had reverted... Should only be the addition of those last three properties. Also, was debating if we even want to include default values for the permissions/umask as the actual 'default' would be taken from the filesystem, and not the conf, so default values are only applied when we enable the permissions stuff, so you should be changing them anyways. Figure wide open (777) permissions as the 'default' was the best, but thought it would be good to get another opinion would be good too. Would that not cover all interesting scenarios? Yeah, it would. This was just the easiest way to go about covering the identifiable data as there are a bunch of places where we are creating files on the fs and seemed a bit overkill to replace all of them for this use case. For creating individual files, its <10 files in src/main for creating files, though a fair number in src/test. However, for creating directories its twice that number and even more for testing. Each of those would need to be wrapped to set the default permissions, doable and starting to think that it's the right way to go about it in the end...hmmm
        Hide
        Lars Hofhansl added a comment -

        What's with all the changes to hbase-defaults.xml?

        For this jira I was actually thinking something simpler: an umask for HBase created directories and an umask for HBase created files. Would that not cover all interesting scenarios?

        Show
        Lars Hofhansl added a comment - What's with all the changes to hbase-defaults.xml? For this jira I was actually thinking something simpler: an umask for HBase created directories and an umask for HBase created files. Would that not cover all interesting scenarios?
        Hide
        Jesse Yates added a comment -

        First iteration, no unit tests, but should cover the identifiable data in hdfs.

        Show
        Jesse Yates added a comment - First iteration, no unit tests, but should cover the identifiable data in hdfs.

          People

          • Assignee:
            Jesse Yates
            Reporter:
            Jesse Yates
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development