----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55246/#review162050 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 196) <https://reviews.apache.org/r/55246/#comment233299> need to check `tPathUpdate == null`. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java (line 141) <https://reviews.apache.org/r/55246/#comment233300> Need to check `tPermUpdate == null` sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 973) <https://reviews.apache.org/r/55246/#comment233353> Do we have any limitation of the `UpdateTransactionBlock` can be used here? Or the order of executing it and `dropSentryRole`? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 978) <https://reviews.apache.org/r/55246/#comment233356> Beyond the logic of handling extra `updateTransactionBlock`, all these functions are the same as the functions they override. Can we extract all the `new TransactionBlock() {...}` together? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1209) <https://reviews.apache.org/r/55246/#comment233355> It might be a naive question. When `pm.makePersistentAll(groups)`, are the `groups` inserted as new recrods with different primary key or update in-place? I did not see `MSentryGroup` has a primary key. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3327) <https://reviews.apache.org/r/55246/#comment233340> If things go wrong, this function will return 0 as last changed ID. You might want to check every return from `getLastProcessedPermChangedID` to handle the case. An alternative is throw the `Exception` out of this function. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3345) <https://reviews.apache.org/r/55246/#comment233342> What if the `permChanges` is empty? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3357) <https://reviews.apache.org/r/55246/#comment233341> Would be nice to be consistent of the parameters between these two functions, either both use `long` or `Long`. I'd suggest to use `long` to avoid `null` parameter. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java (line 173) <https://reviews.apache.org/r/55246/#comment233345> Line 186 has the condition `retryNum >= ...`, which makes this `while (retryNum < ...)` not necessary. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java (line 180) <https://reviews.apache.org/r/55246/#comment233343> we can use ``` } catch (SentryUserException e) { throw e; } catch (Exception e) { retryNum++; .... } ``` sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java (line 191) <https://reviews.apache.org/r/55246/#comment233344> What is the difference between `retryCount` and `retryNum`? Btw, should we move `retryCount.inc()` after `retryNum++`? Otherwise, in the following block it would not update `retryCount.inc()`. ``` if (retryNum >= transactionRetryMax) throw new Exception(...) } ``` sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/UpdateTransactionBlock.java (line 27) <https://reviews.apache.org/r/55246/#comment233346> `{@link TransactionBlock}` sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/UpdateTransactionBlock.java (line 31) <https://reviews.apache.org/r/55246/#comment233347> Can it be `final`? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/UpdateTransactionBlock.java (line 48) <https://reviews.apache.org/r/55246/#comment233348> Do we need an indent here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 248) <https://reviews.apache.org/r/55246/#comment233350> One suggestion is that we can add the following code: ``` Precondition.checkState(sentryPlugins.size() == 1) ``` So that once Sentry supports different plugins, we will not miss the oppurtunity to change here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 445) <https://reviews.apache.org/r/55246/#comment233357> Shouldn't `onDropSentryPrivilege` happen after `dropPrivilege`? What if `dropPrivilege` fails in the middle, but `onDropSentryPrivlege` success? - Lei Xu On Jan. 18, 2017, 1:44 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55246/ > ----------------------------------------------------------- > > (Updated Jan. 18, 2017, 1:44 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 > 9ecd9e4d9a862804eab26594c28aa691b89947aa > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java > a346587fd5c41c826545738faa2f9b95e1d9f0dc > > 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 > 47c9f9dd7105ba5440a81ace9292d730df26f3cd > > 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 > 8e2a6d5fd8f6cdd285d47c66ca7f7e5444965d7d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java > b88e7d17b641ddedaf1d0aa3ce4367e3fd9b3c23 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java > 2ccace01ae5fbbb5450a656ea66dbd517cf320dd > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > dc8fdbfa21d5cb0bbe821c7e354e24dd025a35f6 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 0712e2c9246ba7fe0c81d42861628c60eee2cfa0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java > ec365bfaeab5d4a4fea237db83962570622cd00f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/UpdateTransactionBlock.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 51217408386730d9aa01be03d1c74c71369a6b6e > > 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 > >
