----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55246/#review161292 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 180) <https://reviews.apache.org/r/55246/#comment232549> It seems like there shouldn't be the case where tPathUpdate is null (in fact the constructor used to create such entity can be safely removed) So I suggest for hashCode to simply return tPathUpdate.hashCode() - I don't see any vaue in adding extra 31 to the result. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 194) <https://reviews.apache.org/r/55246/#comment232550> The whole function can be converted to a single return statement sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 211) <https://reviews.apache.org/r/55246/#comment232553> return tPathsUpdate.equals(other.tPathsUpdate); sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java (line 122) <https://reviews.apache.org/r/55246/#comment232554> Same comment - this can be replaced with a single return statement returning tPermUpdate.hashCode(). The constructor that creates null tPermUpdate is never used and can be removed so that there shouldn't be a null tPermUpdate. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java (line 131) <https://reviews.apache.org/r/55246/#comment232555> Same comment - it can be a single return statement sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java (line 156) <https://reviews.apache.org/r/55246/#comment232556> return tPermUpdate.equals(other.tPermUpdate); sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java (line 220) <https://reviews.apache.org/r/55246/#comment232558> Since you are adding the comment, can you clarify what is this in-memory cache and what it is used for? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 166) <https://reviews.apache.org/r/55246/#comment232564> Multi-line TODOs don't work very well with tools like IntelliJ sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 381) <https://reviews.apache.org/r/55246/#comment232580> 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. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 400) <https://reviews.apache.org/r/55246/#comment232579> It may be easier to follow if you rewrite this as if (authzObj == null) { return null } // Regular code 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/#comment232527> final? Also, what is the magic value 5? Will 1 be Ok? 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/#comment232585> This predates your change, but why are we modifying the privilege here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 78) <https://reviews.apache.org/r/55246/#comment232583> This isn't related to your change, but the field isn't used by anything - can you remove it as well> 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/#comment232584> Can you clarify the idea behind having multiple SentryStore plugins? 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/#comment232581> privilegesUpdateMap is always null, what is the idea here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 269) <https://reviews.apache.org/r/55246/#comment232582> privilegesUpdateMap is also always null here as well - Alexander Kolbasov 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 > >
