> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java, > > line 53 > > <https://reviews.apache.org/r/26900/diff/4/?file=728607#file728607line53> > > > > Should'nt this be always same as "sentry.service.security.mode". If so, > > why do not we reuse the same property?
The ServiceConstants class is use both in the client ans server. I did not want the Sentry HDFS client code to depend on any of the core Sentry packages, so I created a separate ServiceConstants class. > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java, > > line 56 > > <https://reviews.apache.org/r/26900/diff/4/?file=728607#file728607line56> > > > > 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? will do > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java, > > line 57 > > <https://reviews.apache.org/r/26900/diff/4/?file=728607#file728607line57> > > > > 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? will do > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java, > > line 59 > > <https://reviews.apache.org/r/26900/diff/4/?file=728607#file728607line59> > > > > Same as above. will do > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, > > line 99 > > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line99> > > > > Consider extending from AbstractTestWithStaticConfiguration (see > > example AbstractMetastoreTestWithStaticConfiguration), so that starting up > > services logic and other utilities can be reused. I initially tried that but I wanted to do stuff like start and stop the sentry service in various permutations during dev-testing.. also.. I think AbstractTestWithStaticConfiguration starts and stops everything using @BeforeClass and @AfterClass.. I initially wanted to it to stop and start before every testCase > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, > > line 102 > > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line102> > > > > Looks like you are not using this class, but instead using > > MiniDFS.PseudoGroupMappingService ? In which case, can we remove this class > > here? Will do > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, > > line 291 > > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line291> > > > > Rename fn name to startDFS, as we are not starting Yarn here? Starting YARN for some reason results in the test going OOM... thought i would look into it later.. > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, > > lines 308-314 > > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line308> > > > > 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. I actually prefer putting the actual property name in tests as much as possible.. That way I dont have to follow the IDE link to know the actual property name.. > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, > > line 315 > > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line315> > > > > Nit: You may want to add a comment, that is can be removed when moved > > to hadoop 2.6, as minidfs defaults to this? will do > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, > > line 322 > > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line322> > > > > Make sure mkdirs succeded before proceeding? will do > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, > > line 483 > > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line483> > > > > Hmm, why does meta store throw an exception the first time? Ive sinced fixed this in the Hive / Metastore Binding.. by adding a retry.. Earlier if Sentry went down and came back up, neither netastore or HS2 was every able to connect to Sentry again.. > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, > > lines 497-499 > > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line497> > > > > Is this repeated on purpose to test idempotency? I have since added more tests.. > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, > > line 529 > > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line529> > > > > It is not clear from the fn name that it also grants select permissions > > to p1_admin which is visisble outside this function. will do > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, > > line 547 > > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line547> > > > > Why are we not running MR here? The MiniMR cluster was going OOM when I start it... will look into it later (did not think it was a priority since HDFS read write is working) > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, > > line 552 > > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line552> > > > > select corresponds to READ_EXECUTE? Not just READ? So POSIX directories apparantly need an execute flag to list (ls) it.. since all tables/dbs are ultimately mapped to directories.. it doesnt make sense to grant READ and not EXECUTE.. Also.. considering we don't technically 'execute' a file in HDFS, we felt it was safe to give all objects an EXECUTE flag as well.. > On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java, > > lines 108-143 > > <https://reviews.apache.org/r/26900/diff/4/?file=728652#file728652line108> > > > > Why was this test commented out? Had added/removed it to test some stuff earlier (before making everything a plugin).. will remove.. - Arun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26900/#review58943 ----------------------------------------------------------- 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 > >
