Thanks for the detailed review lenni.. Will be sending out a patch incorporating feedback shortly..
On Sun, Oct 19, 2014 at 12:14 AM, Lenni Kuff <[email protected]> wrote: > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26900/ > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java > <https://reviews.apache.org/r/26900/diff/1/?file=724996#file724996line242> > (Diff > revision 1) > > public class SentryHiveAuthorizationTaskFactoryImpl implements > HiveAuthorizationTaskFactory { > > 242 > > LOG.debug("## FULL AST : [" + ast.dump() + "]"); > > remove debug logging? > > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java > <https://reviews.apache.org/r/26900/diff/1/?file=724996#file724996line323> > (Diff > revision 1) > > private SentryHivePrivilegeObjectDesc analyzePrivilegeObject(ASTNode ast) > > 323 > > case 880: > > 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/diff/1/?file=724997#file724997line206> > (Diff > revision 1) > > public String get(String varName, String defaultVal) { > > 206 > > Log.warn("Using the deprecated config setting " + > currentToDeprecatedProps.get(varName).getVar() + > > 206 > > // Log.warn("Using the deprecated config setting " + > currentToDeprecatedProps.get(varName).getVar() + > > 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/diff/1/?file=724998#file724998line90> > (Diff > revision 1) > > 90 > > if (tableEvent.getTable().getSd().getLocation() != null) { > > 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/diff/1/?file=724998#file724998line182> > (Diff > revision 1) > > public void onCreateDatabase(CreateDatabaseEvent dbEvent) > > 182 > > // TODO : notify SentryHMSPathCache > > 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/diff/1/?file=724998#file724998line217> > (Diff > revision 1) > > 217 > > // TODO Auto-generated method stub > > 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/diff/1/?file=725005#file725005line20> > (Diff > revision 1) > > 20 > > public interface AuthzPaths { > > 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/diff/1/?file=725006#file725006line22> > (Diff > revision 1) > > 22 > > public interface AuthzPathsDumper<K extends AuthzPaths> { > > 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/diff/1/?file=725008#file725008line33> > (Diff > revision 1) > > 33 > > public class HMSPaths implements AuthzPaths { > > 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/diff/1/?file=725008#file725008line37> > (Diff > revision 1) > > 37 > > path = path.trim(); > > 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/diff/1/?file=725008#file725008line167> > (Diff > revision 1) > > 167 > > StringBuilder sb = new StringBuilder(); > > 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/diff/1/?file=725008#file725008line337> > (Diff > revision 1) > > 337 > > void _addAuthzObject(String authzObj, List<String> authzObjPaths) { > > 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/diff/1/?file=725009#file725009line32> > (Diff > revision 1) > > 32 > > public class HMSPathsSerDe implements AuthzPathsDumper<HMSPaths> { > > 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/diff/1/?file=725009#file725009line50> > (Diff > revision 1) > > 50 > > public TPathsDump createPathsDump() { > > toThrift()? > > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/MetastoreClient.java > <https://reviews.apache.org/r/26900/diff/1/?file=725010#file725010line26> > (Diff > revision 1) > > 26 > > public interface MetastoreClient { > > 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/diff/1/?file=725011#file725011line30> > (Diff > revision 1) > > 30 > > public class PathsUpdate implements Updateable.Update { > > comment on class > > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > <https://reviews.apache.org/r/26900/diff/1/?file=725011#file725011line69> > (Diff > revision 1) > > 69 > > public TPathsUpdate getThriftObject() { > > rename to toThrift()? > > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java > <https://reviews.apache.org/r/26900/diff/1/?file=725013#file725013line53> > (Diff > revision 1) > > 53 > > public class SentryHDFSServiceClient { > > 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/diff/1/?file=725016#file725016line73> > (Diff > revision 1) > > 73 > > if (++counter > 99) { > > 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/diff/1/?file=725037#file725037line32> > (Diff > revision 1) > > 32 > > public class ExtendedMetastoreClient implements MetastoreClient { > > Comment on class. > > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/ExtendedMetastoreClient.java > <https://reviews.apache.org/r/26900/diff/1/?file=725037#file725037line65> > (Diff > revision 1) > > 65 > > retList.add(client.getTable(db.getName(), tblName)); > > 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/diff/1/?file=725037#file725037line80> > (Diff > revision 1) > > 80 > > return client.listPartitions(db.getName(), tbl.getTableName(), > Short.MAX_VALUE); > > 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/diff/1/?file=725038#file725038line31> > (Diff > revision 1) > > 31 > > public class MetastorePlugin extends SentryMetastoreListenerPlugin { > > Comment on class > > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java > <https://reviews.apache.org/r/26900/diff/1/?file=725038#file725038line40> > (Diff > revision 1) > > 40 > > private final AtomicInteger seqNum = new AtomicInteger(5); > > 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/diff/1/?file=725039#file725039line89> > (Diff > revision 1) > > 89 > > * Not implemented for the time being.. > > 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/diff/1/?file=725041#file725041line57> > (Diff > revision 1) > > 57 > > public static volatile SentryPlugin instance; > > 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/diff/1/?file=725042#file725042line42> > (Diff > revision 1) > > 42 > > private final AtomicLong lastSeenSeqNum = new AtomicLong(0); > > 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/diff/1/?file=725042#file725042line56> > (Diff > revision 1) > > 56 > > // UpdateLog is dissabled when updateLogSize = 0; > > disabled > > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java > <https://reviews.apache.org/r/26900/diff/1/?file=725042#file725042line137> > (Diff > revision 1) > > 137 > > public List<K> getAllUpdatesFrom(long seqNum) { > > 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/diff/1/?file=725049#file725049line1470> > (Diff > revision 1) > > private void grantOptionCheck(PersistenceManager pm, String grantorPrincipal, > TSentryPrivilege privilege) > > 1469 > > public Map<String, HashMap<String, String>> retrieveFullPrivilegeImage() { > > 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/diff/1/?file=725050#file725050line160> > (Diff > revision 1) > > private void baseOpen() throws TTransportException { > > 160 > > new TBinaryProtocol(transport), > > 160 > > new TCompactProtocol(transport), > > 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/diff/1/?file=725051#file725051line558> > (Diff > revision 1) > > public TDropPrivilegesResponse drop_sentry_privilege( > > 558 > > // TODO : Sentry - HDFS : Have to handle this > > 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/diff/1/?file=725051#file725051line580> > (Diff > revision 1) > > public TRenamePrivilegesResponse rename_sentry_privilege( > > 580 > > // TODO : Sentry - HDFS : Have to handle this > > 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/diff/1/?file=725054#file725054line167> > (Diff > revision 1) > > public void testDropRole() throws Exception { > > 165 > > // private void waitToCommit(Update hmsCache) throws InterruptedException { > > remove > > > - Lenni Kuff > > On October 17th, 2014, 11:01 p.m. UTC, Arun Suresh wrote: > Review request for sentry, Lenni Kuff, Prasad Mujumdar, and Sravya > Tirukkovalur. > By Arun Suresh. > > *Updated Oct. 17, 2014, 11:01 p.m.* > *Repository: * sentry > Description > > SENTRY-432 : Synchronization of HDFS permissions with Sentry permissions > > Testing > > Manual testing, Basic e2e integration testing, unit tests > > 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) > > View Diff <https://reviews.apache.org/r/26900/diff/> >
