----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55246/#review161215 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 180) <https://reviews.apache.org/r/55246/#comment232451> Here we can just return ``` return tPathsUpdate == null ? 0 : tPathsUpdate.hashCode(); ``` `prime`/`result` does not change the hash code distribution here. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 185) <https://reviews.apache.org/r/55246/#comment232452> Can we merge two `equals(...)` into one `equals(Object that)`? Please refer to Effectiv Java, e.g., http://www.slideshare.net/ibrahimkurce/effective-java-chapter-3-methods-common-to-all-objects Same as the `PermissionUpdates.java`. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 292) <https://reviews.apache.org/r/55246/#comment232453> We can probally `static import Updatable.Update` here. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 316) <https://reviews.apache.org/r/55246/#comment232455> We might want to use format string for logger later, as it eliminates string combaction. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 327) <https://reviews.apache.org/r/55246/#comment232457> 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? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 439) <https://reviews.apache.org/r/55246/#comment232469> Lets not move this. Btw, will there be multi-threading access to `isOutOfSync`? should we protect it? 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/#comment232459> 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. 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/#comment232462> 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. 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/#comment232463> What would be the return if there is an DB/Network error? 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/#comment232461> 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 ? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 460) <https://reviews.apache.org/r/55246/#comment232465> This for loop only keeps the last `update` object? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 468) <https://reviews.apache.org/r/55246/#comment232464> Static function should not be `protected`, it can not be override by child class. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 477) <https://reviews.apache.org/r/55246/#comment232466> This line is too wide :) sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java (line 190) <https://reviews.apache.org/r/55246/#comment232467> These functions might not be necessary to change if we keep two versions of `alterSentryRoleXXX` functions. 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/#comment232468> What is the impact of putting a `null` as value in the map? It looks like that setting `privilegesUpdateMap=null` has the same result. - Lei Xu 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 > >
