----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40957/#review110267 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java (line 115) <https://reviews.apache.org/r/40957/#comment170096> Good point, authzObj should be case insensitive. Set in java uses (o==null ? e==null : o.equals(e)) for uniqueness. And stringObj.equals is a case sensitive check, so looks like we will need to work around this. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java (line 225) <https://reviews.apache.org/r/40957/#comment170082> Comment on this case? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java <https://reviews.apache.org/r/40957/#comment170083> Why do we now not need this? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java (line 287) <https://reviews.apache.org/r/40957/#comment170085> Do we need to retain directories which do not have a authz children? sentry-hdfs/sentry-hdfs-common/src/main/resources/sentry_hdfs_service.thrift (line 54) <https://reviews.apache.org/r/40957/#comment170098> Thinking more about it, this would cause backward incompatibility. We should consider adding an optional field instead of changing an existing optional field in the thrift struct. Also, while we are here. We should also consider thrift version numbers. Some projects bump up the version for every thrift change and some projects only bump it up when there is an incompatible change. Would be good to have a discussion around it and document it on our wiki. sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java (line 290) <https://reviews.apache.org/r/40957/#comment170088> Would this add duplicates? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java (line 1261) <https://reviews.apache.org/r/40957/#comment170091> We should check there are no duplicate acl entries? Thank you Hao for picking this up! Appreciate it! Some comments below. - Sravya Tirukkovalur On Dec. 11, 2015, 8:38 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40957/ > ----------------------------------------------------------- > > (Updated Dec. 11, 2015, 8:38 a.m.) > > > Review request for sentry, Anne Yu, Li Li, and Lenni Kuff. > > > Repository: sentry > > > Description > ------- > > Change-Id: If2dd59c61035ed72a827547ad2f69d3b386417d8 > > In the current design we assume a path can be associated with only one hive > object. But it is possible where a path can be associated with multiple hive > objects: tables/partitions. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/SentryHDFSService.java > 663fe4e3a2318ac32f9afac827cbb2cfb7477af8 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TAuthzUpdateResponse.java > 480c264d90c34806c2a2da3dc86c928e8c5bd8a8 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathChanges.java > 85254d768c3353049a6f13e283bfe501b42e34d2 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathEntry.java > a2a7f7ba947b1b84843f35947c7f802191fbe4bc > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathsDump.java > 200ecad51114da9ae9edc3d64e042dc2e40ffb91 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPathsUpdate.java > d0ee6b6b7b7b414291c54cd120858d0863264aeb > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPermissionsUpdate.java > 850404b7e0b6587b19c6940c087839079240b20f > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegeChanges.java > 76720b98e6e0ffdb7d16da37f10dbd046eff4832 > > sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TRoleChanges.java > 87ef02ddd33c8bbcd4e9926ebd9452b5d4aff99e > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPaths.java > ba16f4ab09df3c997b0ec87c8187b5ded001376b > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java > d52e3617a9d793e6df141d495f7badf3aa754c56 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java > 8f7bb0f61cadffa2390d6915f25cab3a0b406e6d > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > 8c5edd76277c23ba86c8404fd68a3e4c5a530865 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java > ac8459b19ac909b569d227f35f6ec79efdedfc80 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java > b74f9541fc4f48b9e94aa8161eb0a4d2466a468b > > sentry-hdfs/sentry-hdfs-common/src/main/resources/sentry_hdfs_service.thrift > fb60855741e9bff9b841d7e8cb86e6823c4e315f > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java > 29868ae26d512f78b1c8500eabd544f2c4e30e77 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPathsFullDump.java > d01f7dde5b8ed28e79fd257989655a405f3c3776 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java > 4b8a058138f42688512b8d5a9860da90a16d6265 > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java > c9accc116213ce48625ffdd220e00ba634a00d1d > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestMetastoreCacheInitializer.java > f1e729ff9fd94cfbcd9d0ba89d850b665e681904 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > 208c93b77b8d46d87170b2665c5b7e2557812dc7 > > Diff: https://reviews.apache.org/r/40957/diff/ > > > Testing > ------- > > Added e2e test testAuthzObjOnPartitionMultipleTables to do the following: > 1. When two partitions of different tables pointing to the same location with > different grants, ACLs should have union (no duplicates) of both rules. > 2. When alter the table name (tab2 to be tabx), ACLs should remain the same. > 3. When drop a partition that shares the same location with other partition > belonging to the other table, should still have the other table permissions. > 4. When drop a table that has a partition shares the same location with other > partition belonging to other table, should still have the other table > permissions. > > Added e2e test testAuthzObjOnPartitionSameTable to do the following: > 5. When two partitions of the same table pointing to the same location, ACLS > should not be repeated. > > Added e2e test testAuthzObjOnMultipleTables to do the following: > 6. When two tables pointing to the same location, ACLS should have union (no > duplicates) of both rules. > 7. When drop one table shares the same location as the other table, ACLs of > the other table still remain. > > > Thanks, > > Hao Hao > >
