> On May 29, 2014, 8:28 p.m., Ashish Singh wrote: > > shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java, > > line 666 > > <https://reviews.apache.org/r/22016/diff/1/?file=598635#file598635line666> > > > > 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. > >
FsShell serves that purpose. If error it will just return -1 as exitCode, which I log in the helper method. > On May 29, 2014, 8:28 p.m., Ashish Singh wrote: > > shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java, > > line 416 > > <https://reviews.apache.org/r/22016/diff/1/?file=598636#file598636line416> > > > > Same as previous comment regarding settign group. Same > On May 29, 2014, 8:28 p.m., Ashish Singh wrote: > > shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java, > > line 430 > > <https://reviews.apache.org/r/22016/diff/1/?file=598636#file598636line430> > > > > This is similar to Hadoop20FileStatus. Could we push them together into > > a base class? I was heavily-considering, but unfortunately the tree is: Hadoop20Shims (isolated subclass), HadoopShimSecure -> Hadoop20Shims+Hadoop20Shim. So unfortunately the ones that are common in this case (Hadoop20+Hadoop20S) dont form a common tree, so hard to inherit unless I create a proper non-inner class. That's definitely an option if necessary, but I didnt want to invest in it, as Hadoop20Shims should be retired soon. > On May 29, 2014, 8:28 p.m., Ashish Singh wrote: > > shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java, > > line 538 > > <https://reviews.apache.org/r/22016/diff/1/?file=598637#file598637line538> > > > > Same as previous comment on setting group. Same response. > On May 29, 2014, 8:28 p.m., Ashish Singh wrote: > > ql/src/java/org/apache/hadoop/hive/ql/processors/DeleteResourceProcessor.java, > > line 21 > > <https://reviews.apache.org/r/22016/diff/1/?file=598634#file598634line21> > > > > Is this here by mistake? Somehow it came , as I didnt rebase the patch with latest trunk. > On May 29, 2014, 8:28 p.m., Ashish Singh wrote: > > shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java, > > line 524 > > <https://reviews.apache.org/r/22016/diff/1/?file=598637#file598637line524> > > > > 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? Done - Szehon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22016/#review44272 ----------------------------------------------------------- On May 29, 2014, 10:49 p.m., Szehon Ho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22016/ > ----------------------------------------------------------- > > (Updated May 29, 2014, 10:49 p.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 > 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 > bcb2660 > 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 > >
