----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22016/#review44272 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/processors/DeleteResourceProcessor.java <https://reviews.apache.org/r/22016/#comment78625> Is this here by mistake? shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java <https://reviews.apache.org/r/22016/#comment78630> Shouldn't we be trying to change permission even if we fail to set group? Also, if I remember correctly, trying to set group was leading to exceptions. shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java <https://reviews.apache.org/r/22016/#comment78633> Same as previous comment regarding settign group. shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java <https://reviews.apache.org/r/22016/#comment78635> This is similar to Hadoop20FileStatus. Could we push them together into a base class? shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java <https://reviews.apache.org/r/22016/#comment78637> Objects.equal(conf.get("dfs.namenode.acls.enabled"), "true") is used at more than one place. Should we extract it out to a static method? shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java <https://reviews.apache.org/r/22016/#comment78636> Same as previous comment on setting group. - Ashish Singh On May 29, 2014, 8:14 a.m., Szehon Ho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22016/ > ----------------------------------------------------------- > > (Updated May 29, 2014, 8:14 a.m.) > > > Review request for hive. > > > Bugs: HIVE-7119 > https://issues.apache.org/jira/browse/HIVE-7119 > > > Repository: hive-git > > > Description > ------- > > This completes the permission inheritance story, by also inheriting the new > concept of extended ACL's in HDFS from parent. > > This is a bit tricky because only Hadoop 2.4 has extended ACL's. My strategy > is to use the HadoopShims, and only in Hadoop23Shims to have code dealing > with extended ACL's, and then only if the flag "dfs.namenode.acls.enabled" is > true. > > It was also tricky as the main Hive code cannot refer to the HDFS ACL classes > (aclStatus and aclEntry). So made some wrapper API in the shims called > 'hdfsFileStatus' that encompasses both normal file status , and Aclstatus if > acl's are enabled. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/common/FileUtils.java ee61350 > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/FolderPermissionBase.java > PRE-CREATION > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestExtendedAcls.java > PRE-CREATION > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java > 4f566d2 > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 3417474 > > ql/src/java/org/apache/hadoop/hive/ql/processors/DeleteResourceProcessor.java > 83fadeb > shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java > 7aae689 > shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java > e6493eb > shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java > 176f9ae > > shims/common-secure/src/main/java/org/apache/hadoop/hive/shims/HadoopShimsSecure.java > c27df64 > shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java > 02260ce > > Diff: https://reviews.apache.org/r/22016/diff/ > > > Testing > ------- > > For testing, refactored TestFolderPermission into a base + two tests > (TestFolderPermission to test traditional permission without acl's, and > TestExtendedAcl's to test acls). > > > Thanks, > > Szehon Ho > >
