----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26900/#review58943 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java <https://reviews.apache.org/r/26900/#comment100149> Should'nt this be always same as "sentry.service.security.mode". If so, why do not we reuse the same property? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java <https://reviews.apache.org/r/26900/#comment100162> Can we add a comment that this is property for ugi is only for testing purposes, for JAAS based tests if we happen to add them in future? Outside of tests this will never be used isnt it? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java <https://reviews.apache.org/r/26900/#comment100152> Same as above, if this always has to be sentry service's principal, why not reuse the same property? So that users do not have two parameters to set? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java <https://reviews.apache.org/r/26900/#comment100153> Same as above. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java <https://reviews.apache.org/r/26900/#comment100154> Same as above sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100164> Consider extending from AbstractTestWithStaticConfiguration (see example AbstractMetastoreTestWithStaticConfiguration), so that starting up services logic and other utilities can be reused. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100146> Looks like you are not using this class, but instead using MiniDFS.PseudoGroupMappingService ? In which case, can we remove this class here? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100163> There is no property called: sentry.hive.provider. Again try to use constants exposed by services rather than hardcoding. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100160> Rename fn name to startDFS, as we are not starting Yarn here? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100155> Consider using service constants rather than harcoding the parameter value, so that it would be easier to navigate the usages as well as refactor if need to. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100157> Nit: You may want to add a comment, that is can be removed when moved to hadoop 2.6, as minidfs defaults to this? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100158> Make sure mkdirs succeded before proceeding? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100159> Nit: Remove comment Btw, was there a problem running miniMR? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100145> Looks like these local variables, "properties" and "sentryConf", are never used after setting here? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100179> Hmm, why does meta store throw an exception the first time? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100182> Is this repeated on purpose to test idempotency? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100176> It is not clear from the fn name that it also grants select permissions to p1_admin which is visisble outside this function. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100175> Why are we not running MR here? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java <https://reviews.apache.org/r/26900/#comment100174> select corresponds to READ_EXECUTE? Not just READ? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java <https://reviews.apache.org/r/26900/#comment100142> Why was this required? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java <https://reviews.apache.org/r/26900/#comment100143> Why was this test commented out? Thanks for the patch Arun! I just reviewed the test portions of it for now and added some comments below. - Sravya Tirukkovalur On Oct. 22, 2014, 6:20 a.m., Arun Suresh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26900/ > ----------------------------------------------------------- > > (Updated Oct. 22, 2014, 6:20 a.m.) > > > Review request for sentry, Lenni Kuff, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > SENTRY-432 : Synchronization of HDFS permissions with Sentry permissions > > > Diffs > ----- > > pom.xml e172e92 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java > f38ee91 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java > 4d2a625 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java > 38bf8b2 > sentry-dist/pom.xml cd7126b > sentry-dist/src/main/assembly/bin.xml 258e63c > sentry-dist/src/main/assembly/sentry-hdfs.xml PRE-CREATION > sentry-hdfs/pom.xml PRE-CREATION > sentry-hdfs/sentry-hdfs-common/.gitignore PRE-CREATION > sentry-hdfs/sentry-hdfs-common/pom.xml PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPaths.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPathsDumper.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPermissions.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/MetastoreClient.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPathsFullDump.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java > PRE-CREATION > sentry-hdfs/sentry-hdfs-common/src/test/resources/hdfs-sentry.xml > PRE-CREATION > sentry-hdfs/sentry-hdfs-dist/pom.xml PRE-CREATION > sentry-hdfs/sentry-hdfs-dist/src/main/assembly/all-jar.xml PRE-CREATION > sentry-hdfs/sentry-hdfs-namenode-plugin/.gitignore PRE-CREATION > sentry-hdfs/sentry-hdfs-namenode-plugin/pom.xml PRE-CREATION > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationConstants.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryUpdater.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/MockSentryAuthorizationProvider.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/SentryAuthorizationInfoX.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java > PRE-CREATION > sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/resources/hdfs-sentry.xml > PRE-CREATION > sentry-hdfs/sentry-hdfs-service/.gitignore PRE-CREATION > sentry-hdfs/sentry-hdfs-service/pom.xml PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/ExtendedMetastoreClient.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestUpdateForwarder.java > PRE-CREATION > sentry-provider/sentry-provider-db/pom.xml b4167e4 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryMetastoreListenerPlugin.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java > b66037a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 017cf08 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 65905f5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > b54e12e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 40e8a0e > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyStoreProcessor.java > 46f8fb8 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java > e5238a6 > sentry-tests/sentry-tests-hive/pom.xml 10415fc > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/StaticUserGroup.java > 66f088f > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java > 45d24f9 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java > 8ce78bc > > Diff: https://reviews.apache.org/r/26900/diff/ > > > Testing > ------- > > Manual testing, Basic e2e integration testing, unit tests > > > Thanks, > > Arun Suresh > >
