----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26900/#review57290 -----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java <https://reviews.apache.org/r/26900/#comment97872> remove debug logging? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java <https://reviews.apache.org/r/26900/#comment97873> why the integer token types? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java <https://reviews.apache.org/r/26900/#comment97874> Why comment this out? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java <https://reviews.apache.org/r/26900/#comment97875> When would the location be null? Also, create a utility method (if one doesn't already exist) to build the fully qualified table name given a Table. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java <https://reviews.apache.org/r/26900/#comment97876> Seems like we need to handle the ALTER case. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java <https://reviews.apache.org/r/26900/#comment97877> remove "Auto-generated method stub" TODOs here and elsewhere. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPaths.java <https://reviews.apache.org/r/26900/#comment97878> Add brief comment on this public interface and of the methods. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPathsDumper.java <https://reviews.apache.org/r/26900/#comment97879> Does this need to be an interface or can it just be a class? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java <https://reviews.apache.org/r/26900/#comment97880> Comment on this data structure, how is is used, thread safety, etc. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java <https://reviews.apache.org/r/26900/#comment97881> Can you just do this without your own path splitting logic? File file = new File(path); if (!file.isAbsolutePath()) throw ... do { paths.add(file.getName()); file = file.getParent(); } while (file.getParent != null) return paths; sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java <https://reviews.apache.org/r/26900/#comment97882> return Joiner.on(Path.SEPARATOR).join(paths); sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java <https://reviews.apache.org/r/26900/#comment97883> private void and remove the "_" prefix. Here and below. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsSerDe.java <https://reviews.apache.org/r/26900/#comment97884> Shoudl this really be called a SerDe? That has a very specific meaning sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsSerDe.java <https://reviews.apache.org/r/26900/#comment97885> toThrift()? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/MetastoreClient.java <https://reviews.apache.org/r/26900/#comment97886> Why all the interfaces instead of just using classes? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java <https://reviews.apache.org/r/26900/#comment97888> comment on class sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java <https://reviews.apache.org/r/26900/#comment97887> rename to toThrift()? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java <https://reviews.apache.org/r/26900/#comment97889> A lot of this code looks similar to the SentryServiceClient. Can you share more code? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java <https://reviews.apache.org/r/26900/#comment97891> Make 99 a constant and comment on this behavior someplace. Does it make a significant difference to add this occassional releasing of the lock? I think it could cause problems (for example - let's say you unlock and return for some reason you will attempt to unlock again. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/ExtendedMetastoreClient.java <https://reviews.apache.org/r/26900/#comment97892> Comment on class. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/ExtendedMetastoreClient.java <https://reviews.apache.org/r/26900/#comment97894> Is this ever sent over the wire? Seems like it might not scale so well sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/ExtendedMetastoreClient.java <https://reviews.apache.org/r/26900/#comment97893> What if a table has > 32K partitions? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java <https://reviews.apache.org/r/26900/#comment97895> Comment on class sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java <https://reviews.apache.org/r/26900/#comment97896> Comment on how sequence numbers are used. Also, would it be easy to have the initial value be 0 or -1 and have the first update start at 0 or 1? That might make things a bit easier to understand. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java <https://reviews.apache.org/r/26900/#comment97897> What are the implications of not implementing this? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java <https://reviews.apache.org/r/26900/#comment97898> Why is this volatile? Should you use an AtomicReference instead? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java <https://reviews.apache.org/r/26900/#comment97901> Why are these Atomic? Is it for thread safety? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java <https://reviews.apache.org/r/26900/#comment97899> disabled sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java <https://reviews.apache.org/r/26900/#comment97900> Consider using a Tree data structure which has a lot of this functionality built in. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/26900/#comment97902> comment on wha tthis is returning (ie - what are the key/values of these maps) sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java <https://reviews.apache.org/r/26900/#comment97903> We have to be careful about changing this because it needs to be done to be done on the client and server side together. I think there was some config option for this, no? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java <https://reviews.apache.org/r/26900/#comment97904> Address this TODO? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java <https://reviews.apache.org/r/26900/#comment97905> Address this TODO? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java <https://reviews.apache.org/r/26900/#comment97906> remove - Lenni Kuff On Oct. 17, 2014, 11:01 p.m., Arun Suresh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26900/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2014, 11:01 p.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/pom.xml e72b370 > > 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/HMSPathsSerDe.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-service-client/pom.xml PRE-CREATION > 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 > >
