> On Jan. 11, 2017, 1:21 p.m., Lei Xu wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java, > > line 327 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600435#file1600435line327> > > > > Could you explain why here we create a local map, as the content of > > this local map will not be returned to the caller, it seems not have any > > effect? > > > > Btw, what is the line width limitation used in Sentry project?
The logic here is not correct, will fix it. > On Jan. 11, 2017, 1:21 p.m., Lei Xu wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 482 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600438#file1600438line482> > > > > Is there any concurrent updates? > > > > Say two updates come at the same time, > > `getLastProcessedPermChangeIdCore(pm)` would return the same `last change > > id`? So that two different updates will be persisted with the same change > > Id? > > > > If we just want to increase change id monotonically, we can just > > increase it within the persistent transaction. There could be concurrent updates. The changeID is a primary key. DB does not allow duplicate entry for that. So the transcation will fail and retry. And note that everything within TransactionBlock is in one transaction. > On Jan. 11, 2017, 1:21 p.m., Lei Xu wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java, > > line 446 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600435#file1600435line446> > > > > Lets not move this. > > > > Btw, will there be multi-threading access to `isOutOfSync`? should we > > protect it? Sure I can keep it. Wondering why you think it is a bad idea to remove? setOutOfSync is currently not used in anywhere, we may need to refactor this code later. > On Jan. 11, 2017, 1:21 p.m., Lei Xu wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 3217 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600438#file1600438line3217> > > > > What would be the return if there is an DB/Network error? If the transaction is not successful, it will retry. And after a certain number stop retries throws exception. > On Jan. 11, 2017, 1:21 p.m., Lei Xu wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 3278 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600438#file1600438line3278> > > > > Should'n we re-throw this exception? > > > > Otherwise, the database error is swallowed here. > > > > Btw, my local git repo can not find `MSentryPathChange` class. Is it a > > generated class ? Yeah, make sense. Will change it. MSentryPathChange is just committed by SENTRY-1538. > On Jan. 11, 2017, 1:21 p.m., Lei Xu wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 444 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600438#file1600438line444> > > > > If we keep the old signature of the function, and add the new version > > as an override: > > > > ``` > > public void alterSentryRoleGrantPrivilege(String grantorPrincipal, > > String roleName, TSentryPrivilege privilege) throws Exception { > > alterSentryRoleGrantPrivileg(grantorPrinciple, roleName, > > privilege, null); > > } > > ``` > > > > It might be able to reduce the patch size. May I ask is there a limitaion of size of a patch? > On Jan. 11, 2017, 1:21 p.m., Lei Xu wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, > > line 465 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600439#file1600439line465> > > > > This for loop only keeps the last `update` object? yeah, please note the explanation in TODO. > On Jan. 11, 2017, 1:21 p.m., Lei Xu wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java, > > line 641 > > <https://reviews.apache.org/r/55246/diff/2/?file=1600441#file1600441line641> > > > > What is the impact of putting a `null` as value in the map? It looks > > like that setting `privilegesUpdateMap=null` has the same result. yeah it is the same thing. - Hao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55246/#review161215 ----------------------------------------------------------- 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 > >
