> On Jan. 12, 2017, 1:35 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 106 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600438#file1600438line106> > > > > final? Also, what is the magic value 5? Will 1 be Ok?
I keep the original INIT_CHANGE_ID from SentryPlugin here for now. It looks like anything larger that 0 should be fine. Refer to NO_LAST_SEEN_HMS_PATH_SEQ_NUM. > On Jan. 12, 2017, 1:35 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, > > line 261 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600439#file1600439line261> > > > > Can you clarify the idea behind having multiple SentryStore plugins? Unfortunately, I am not exactly aware of why there may have multiple SentryStore plugins. My guess is for future feature other than HDFS sync, If any seperate logic need to be added to Sentryservice, it could be done by adding a new plugins. > On Jan. 12, 2017, 1:35 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, > > line 265 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600439#file1600439line265> > > > > privilegesUpdateMap is always null, what is the idea here? This is not correct, fixing it. > On Jan. 12, 2017, 1:35 a.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java, > > line 381 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600435#file1600435line381> > > > > What is the point in this map - it is never returned back, so why > > create it in the first place? > > > > Looking at the caller it seems to be always null when passed in, so I > > don't quite understand what is going on here. This is not correct, fixing it. > On Jan. 12, 2017, 1:35 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 490 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600438#file1600438line490> > > > > This predates your change, but why are we modifying the privilege here? e.g. For priviege with action ALL, if revoke priviege with action Insert, the role should still have priviege with action Select. - Hao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55246/#review161292 ----------------------------------------------------------- On Jan. 10, 2017, 2:31 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55246/ > ----------------------------------------------------------- > > (Updated Jan. 10, 2017, 2:31 a.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and > Vamsee Yarlagadda. > > > Repository: sentry > > > Description > ------- > > SENTRY-1536: Refactor SentryStore transaction management to allow for extra > transactions for a single permission update > > Change-Id: I0571ca25bd8cc20b137baa5b840542f9ef8e7347 > > To persist single permission change, it needs to combine multiple things in a > single transaction: > 1. Doing the actual operation (priv change) > 2. Updating notification ID. > > All the above steps are put into in a single transaction to guarantee that > notificationID handling is atomic. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > 7cfb3bf57bd1a425b07df6c08db31b9691dd17f5 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java > 98349232bc658c39791e58b64949ecb975fff7a0 > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java > 24729282bf17960152f87b1d3124caeafb47e6b2 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > 3695709e03e683afe6196def53883e37e4910a1c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java > 2ff715f66ea6c2589a281b988438526546af3d3b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > e678b575f86cd4797ad01f12e4a60fbeec9f84f5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 8789a48c47e439bc2791cfe5a3b716b586025a7a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > caa3c58b6d2e5874bea52379b9dd549a76698b9b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > f3cefd6a232bfb91db28f04bebcc98ab3c1ca658 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > a35c8d7dde485cf46d61968a211d1dbb6d9d6076 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java > 1c3a4f29984379f5246da8d85fe661320c8a1043 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > d601b1e107ab3c3a4d9cc5e3038a11547182c5c9 > > Diff: https://reviews.apache.org/r/55246/diff/ > > > Testing > ------- > > New unit tests added in TestHMSFollower. > > > Thanks, > > Hao Hao > >
