> On Oct. 21, 2014, 5:08 p.m., Lenni Kuff wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java, > > line 27 > > <https://reviews.apache.org/r/26900/diff/3/?file=727086#file727086line27> > > > > 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();
I agree.. Exposing synchronization primitives is definitely an anti-pattern.. So our thinking was, in the namenode side, we can get 2 types of updates from a single RPC call, a Paths update and a Permissions updates. Sometimes these updates are corellated.. which means we would want both the updates to happen within the same lock context.. which means the Implementing classes cannot manage the locksThus we pass an external lock... I agree I should do a better job at docing this.. (Me and tucu had 2 weeks of back and forth for this one.. this is what we finally came up with) > On Oct. 21, 2014, 5:08 p.m., Lenni Kuff wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java, > > line 46 > > <https://reviews.apache.org/r/26900/diff/3/?file=727086#file727086line46> > > > > We should not make a lock as part of the interface. Exposing locking > > external to the class will get us in trouble. See above comment.. > On Oct. 21, 2014, 5:08 p.m., Lenni Kuff wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java, > > line 68 > > <https://reviews.apache.org/r/26900/diff/3/?file=727087#file727087line68> > > > > We shouldn't be exposing the locking behavior externally, ie we should > > not take a lock as a parameter. See above comment.. > On Oct. 21, 2014, 5:08 p.m., Lenni Kuff wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java, > > line 69 > > <https://reviews.apache.org/r/26900/diff/3/?file=727087#file727087line69> > > > > 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. We actually do use the readlock.. It is in SentryAuthorizationInfo class .. used in the namenode plugin.. It manages the locks.. > On Oct. 21, 2014, 5:08 p.m., Lenni Kuff wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java, > > line 90 > > <https://reviews.apache.org/r/26900/diff/3/?file=727087#file727087line90> > > > > What if it doesn't have 2? Should this be a Precondition check? So.. this is a special case for table/partition renames (I had mentioned it in a comment prior to the If..) All Path partial updates (except for table/partition rename) will have exactly 1 pathChange so this should not go into a Precondition.. > On Oct. 21, 2014, 5:08 p.m., Lenni Kuff wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java, > > line 125 > > <https://reviews.apache.org/r/26900/diff/3/?file=727090#file727090line125> > > > > add precondition check check to ensure addPrivs and delPrivs are of > > equal size? Is that always expected to be true? See previous comment.. Yes.. for alter table/partion rename, both add and del will have exactly 1 change.. > On Oct. 21, 2014, 5:08 p.m., Lenni Kuff wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java, > > line 35 > > <https://reviews.apache.org/r/26900/diff/3/?file=727092#file727092line35> > > > > Explain behavior when the update to sentry fails (ie Sentry Service is > > down). Will be fixing this up.. when I remove dependency from Sentry -> Hive > On Oct. 21, 2014, 5:08 p.m., Lenni Kuff wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java, > > line 89 > > <https://reviews.apache.org/r/26900/diff/3/?file=727095#file727095line89> > > > > What is the action for the user/support team when they see this warning > > message? This is more of a dev log.. i guess > On Oct. 21, 2014, 5:08 p.m., Lenni Kuff wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java, > > line 273 > > <https://reviews.apache.org/r/26900/diff/3/?file=727095#file727095line273> > > > > 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. The seqNum is just to tag the Update object... creating any update object requies a seqNum - Arun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26900/#review57598 ----------------------------------------------------------- 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 > >
