----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40807/#review108769 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java (line 122) <https://reviews.apache.org/r/40807/#comment168223> do we also need to exclude the empty string here? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java (line 123) <https://reviews.apache.org/r/40807/#comment168224> how about move (authzObj != null) to addAuthzObjs function and just call addAuthzObjs(authzObj)? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java (line 133) <https://reviews.apache.org/r/40807/#comment168226> how about move the verification logic to addAuthzObjs function and just call addAuthzObjs(authzObjs)? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java (line 272) <https://reviews.apache.org/r/40807/#comment168240> Is it safe to return the original set in public method here? Should we return a copy of authzObjs? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java (line 315) <https://reviews.apache.org/r/40807/#comment168242> is it possible that authzObjs is null here and also in line 490? eg. clearAuthzObjs() had been called sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java (line 73) <https://reviews.apache.org/r/40807/#comment168244> how about add a public method for the size of authzObjs in Entry? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java (line 1178) <https://reviews.apache.org/r/40807/#comment168249> should we delete the hdfs dir and tbls/db after this test? - Li Li On Dec. 1, 2015, 1:44 a.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40807/ > ----------------------------------------------------------- > > (Updated Dec. 1, 2015, 1:44 a.m.) > > > Review request for sentry. > > > Bugs: SENTRY-953 > https://issues.apache.org/jira/browse/SENTRY-953 > > > Repository: sentry > > > Description > ------- > > 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. I removed the thrift generated code form the > review to avoid noise. > > > Diffs > ----- > > > 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/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/TestUpdateableAuthzPaths.java > 4b8a058138f42688512b8d5a9860da90a16d6265 > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java > c9accc116213ce48625ffdd220e00ba634a00d1d > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > 208c93b77b8d46d87170b2665c5b7e2557812dc7 > > Diff: https://reviews.apache.org/r/40807/diff/ > > > Testing > ------- > > Added a simple test, but need more test coverage. Following is the test plan: > 1.1. Two partitions of different tables pointing to same location with > different grants => ACLS should have union (no duplicates) of both rules. > 1.2. Drop first table => should still have second table permissions > 1.3. Drop second table => should still have first table permissions > 1.4. Do 1.2 but drop partition instead > 1.5. Do 1.3 but drop partition instead > 2.1. Two partitions of same table pointing to same location => ACLS should > not be repeated. > 2.2. Drop first partition => Should still have acls > 2.3. Same as 2.2, but drop second partition > 3.1. Two tables pointing to same location => union of rules. > 3.2. Drop first table > 3.3. Drop second table > One thing I cannot test on pseudo cluster is initialization is happening > correctly when there are multiple objects pointing to the same path as there > is no way to persist meta store and restart HMS. I will try to mock is some > how. > > > Thanks, > > Sravya Tirukkovalur > >
