----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26900/#review57598 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java <https://reviews.apache.org/r/26900/#comment98375> remove sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java <https://reviews.apache.org/r/26900/#comment98377> Please add this to the configuration document (along with any other missing configs). It would be good to also include their default values. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java <https://reviews.apache.org/r/26900/#comment98392> Let's talk about this. I think we should be able to handle this in a different manner and also make it so we don't expose the synchronization internals (which shouldn't be in the interface). For example, can you have a class that takes takes an "update" as a param in the ctor and does all the synchronization internally? One example might be: abstract class BaseClass updatePartitial() { lock updatePartialImpl(); unlock } abstract updatePartialImpl(); sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java <https://reviews.apache.org/r/26900/#comment98383> We should not make a lock as part of the interface. Exposing locking external to the class will get us in trouble. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java <https://reviews.apache.org/r/26900/#comment98381> Please add a brief comment on what this is for. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java <https://reviews.apache.org/r/26900/#comment98382> We shouldn't be exposing the locking behavior externally, ie we should not take a lock as a parameter. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java <https://reviews.apache.org/r/26900/#comment98394> It doesn't look like you ever use the readLock(), so why not keep in simple and just use a regular lock object? Or Simplify it even more and make the function synchronized? Unless we know there is a huge contention issue, let's keep it as simple as possible and then optimize for performance later. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java <https://reviews.apache.org/r/26900/#comment98387> What if it doesn't have 2? Should this be a Precondition check? sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java <https://reviews.apache.org/r/26900/#comment98395> add precondition check check to ensure addPrivs and delPrivs are of equal size? Is that always expected to be true? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java <https://reviews.apache.org/r/26900/#comment98396> Explain behavior when the update to sentry fails (ie Sentry Service is down). sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java <https://reviews.apache.org/r/26900/#comment98398> Will this ever get retried? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java <https://reviews.apache.org/r/26900/#comment98400> What is the action for the user/support team when they see this warning message? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java <https://reviews.apache.org/r/26900/#comment98401> Why does creating a full impage update require passing a sequence number? Is it because it creates a full impage up to the given seq number? Consider rename to createFullImgTo(long seqNum); or something and/or comment what happens on the interface. - Lenni Kuff On Oct. 21, 2014, 11:03 a.m., Arun Suresh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26900/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2014, 11:03 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 > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java > dfcf63a > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java > 0c8778b > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java > 316530b > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPaths.java > 9ea50c7 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java > e445634 > > 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/HMSPathsSerDe.java > b642a3b > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/MetastoreClient.java > 3b64756 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > c5ac783 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java > c9ed96e > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java > fa31a19 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 397a534 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java > 1649ffc > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java > 165892d > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPathsFullDump.java > dcd70c1 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java > 9d0d366 > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java > 90192dc > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/ExtendedMetastoreClient.java > c0358f4 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java > 17f15f0 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java > ab07494 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > 5bb6d45 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java > b0fc5ed > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryMetastoreListenerPlugin.java > 2249940 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java > b3cb487 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > ae8db7d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > afc92d1 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java > 9ba4aa1 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > 26cf978 > > Diff: https://reviews.apache.org/r/26900/diff/ > > > Testing > ------- > > Manual testing, Basic e2e integration testing, unit tests > > > Thanks, > > Arun Suresh > >
