----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26900/#review57294 -----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java <https://reviews.apache.org/r/26900/#comment97918> Will do sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java <https://reviews.apache.org/r/26900/#comment97917> So when I started this feature, There were a bunch of changes happeneing on the Hive side.. more specifically the HiveParses constants.. Switching the jar files also did not solve this, since static constants were being inlined in the compiled class files.. So this is an interim fix.. which we will remove in the final patch.. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java <https://reviews.apache.org/r/26900/#comment97919> If you notice, the original file, due to a typo, was using 'Log' (which is a jetty 'Log' class instead of the 'LOG' static variable).. which was bringing in wierd jetty dependencies.. so just commented it out for the time being.. WIll uncomment and fix the typo.. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java <https://reviews.apache.org/r/26900/#comment97921> Yup... Looks like Hive sets it to null for some cases.. I had considered refactoring into a separate method.. unfortunately, if you notice the event object passed to callback methods are all different type (DropTableEvent / CreateTableEvent) etc.. so it was not possible to write a single utility function.. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java <https://reviews.apache.org/r/26900/#comment97923> Yup.. agreed.. I needed some help with this from someone with more experience with HIVE sql (Since I am not sure about of all the possible ALTER table cases).. I was thinking we could flesh this out as and when we hit cases during testing.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPaths.java <https://reviews.apache.org/r/26900/#comment97924> Will do.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPathsDumper.java <https://reviews.apache.org/r/26900/#comment97925> Hmmm.. i agreed it could have been some sort of utility class, but AuthzPaths class hierarchy made this a bit more complicated.. If you don't mind.. I feel we should keep it as it is.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java <https://reviews.apache.org/r/26900/#comment97926> Will do.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java <https://reviews.apache.org/r/26900/#comment97927> We wanted to avoid using the '.split()' method since it uses regexes and not particularly efficient.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java <https://reviews.apache.org/r/26900/#comment97928> Agreed... Joiner would have been cleaner But would also lead to more object creation.. and tucu was very particular about not using it in any code running on the Namenode. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java <https://reviews.apache.org/r/26900/#comment97929> If you notice there is another method : addAuthzObject(String authzObj, List<List<String>> authzObjPathElements) In the compile stage, due to type erasure, it will end up having the same signature as : addAuthzObject(String authzObj, List<String> authzObjPaths) so I was forced to change the method name and since it was not in the public interface anyway, I added '_' to it.. Also, i am using it in some of the testcases, so connot make it private.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsSerDe.java <https://reviews.apache.org/r/26900/#comment97930> Will do.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsSerDe.java <https://reviews.apache.org/r/26900/#comment97931> Will do.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsSerDe.java <https://reviews.apache.org/r/26900/#comment97932> Hmmm... Id prefer keeping it as 'createPathsDump' since the interface API method has not idea about thrift.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/MetastoreClient.java <https://reviews.apache.org/r/26900/#comment97933> So, when We started work on this, there was a discussion about possible changes in the existing MetastoreClient to handle this.. We decided that during developement, having an interface that exposes just the required method would let us isolate the required changes better.. and at the same time decouple the Sentry HDFS codebase from the Hive metastore codebase.. I shall expain with a specific case where we might need changes to Hive Metastore client and why using the interface helped.. as a reponse to one of the later comments you made.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java <https://reviews.apache.org/r/26900/#comment97934> Will do.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java <https://reviews.apache.org/r/26900/#comment97935> Will do.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java <https://reviews.apache.org/r/26900/#comment97936> Agreed.. Thing is, a lot of the common stuff is boiler-plate stuff to handle Kerberos and Ugi.. I could have pull it out and put it in a common package/project.. But I wanted to : 1) keep the Sentry HDFS codebase as separate as possible wrt to the core Sentry code base 2) Make minimal changes to the existing codebase.. 3) If you notice, the Client/Server Config classes used here are different for sentry hdfs and core sentry.. so pulling this out would be a bit more involved.. Given the above, I felt copy-pasting and making minor changes would be easier.. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java <https://reviews.apache.org/r/26900/#comment97937> Agreed.. will refactor into a constant.. wrt. removing the counter.. Just because it is running on the namenode, I still feel it warrents a place here.. I don't think the unlock case will cause problems.. since this is a Reentrant lock.. so multiple (un)locks wont cause issues sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/ExtendedMetastoreClient.java <https://reviews.apache.org/r/26900/#comment97938> Will do.. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/ExtendedMetastoreClient.java <https://reviews.apache.org/r/26900/#comment97939> Agreed.. Like I mentioned in an earlier comment.. This is possibly some change we need to make on the Hive metastore client code.. As far as I know, no current API exists on Metastore to handle this sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/ExtendedMetastoreClient.java <https://reviews.apache.org/r/26900/#comment97940> See previous comment.. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java <https://reviews.apache.org/r/26900/#comment97941> Will do.. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java <https://reviews.apache.org/r/26900/#comment97942> I wanted to ensure that client code (irrespective of whether it is initialized to 0 or -1) somehow detects this is a first update.. But looks like this is not required anymore.. will fix if it does not cause any problems.. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java <https://reviews.apache.org/r/26900/#comment97909> Was meant to be used by Hue (for eg : Given a path, it canfigure out what is the associated table/db and ALL other partition associated with it) It can also be used to provide better hints for creating input splits for MR tasks. We can remove this for the time being.. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java <https://reviews.apache.org/r/26900/#comment97910> The initialization and use of the variables can be in different threads. Technically, Atomic refernce and volatile are no different.. I'd prefer volatile to Atomic refernce since its more a language feature than library. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java <https://reviews.apache.org/r/26900/#comment97913> Yup... The updates come in from one thread and requests for the udate from another. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java <https://reviews.apache.org/r/26900/#comment97914> We had considered using a Tree initially. Reason we stuck to List is basically because Treas are better for searches. Lists are better for updates and this is end of the day an update log (addition to the tail/head of list is constant time and tree is o(logn)) I also plan to keep the update log size < 50 so search times for the current seqNum using a sorted treemap would not be a significant improvement over a iterator scan to the current seqNum to warrant the overhead of the tree datastructure.. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/26900/#comment97916> It is basically : AuthZObj(db/table) -> (Role -> permission) I will add it comments.. thanks for pointing it out.. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java <https://reviews.apache.org/r/26900/#comment97915> I agree.. I did a full test (I ran all the e2e tests in Sentry hive and solr) with TCompactProtocal and everything looked fine.. We also control the client code. Only thing we have to ensure is that Impala and Hive use the correct version of Sentry.. - Arun Suresh 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 > >
