----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55246/#review163915 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 52) <https://reviews.apache.org/r/55246/#comment235434> Is this documentation still accurate or it should be updated? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 116) <https://reviews.apache.org/r/55246/#comment235437> Are these still relevant? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 292) <https://reviews.apache.org/r/55246/#comment235441> I think it may be cleaner here and below to just return an update and let the caller convert it to DeltaTransactionBlock right before passing to SentryStore. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 300) <https://reviews.apache.org/r/55246/#comment235439> Do we still need to call handleUpdateNotification()? What will it do in the new world? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3298) <https://reviews.apache.org/r/55246/#comment235443> Do we actually use it for anything other than testing? In practice - do we really want to get the last changeID or the actual delta associated with that ID? This may affect your interfaces here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3313) <https://reviews.apache.org/r/55246/#comment235444> Is it used for anything rather then tests? Do you want to get perm change for any ID or just for the last one? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3324) <https://reviews.apache.org/r/55246/#comment235447> It is better to say what went wrong rather than wha is expected. For example. It is also sueful to provide extra available information like actual ID causing the problem. "inconsistent permission delta: %d permissions for the same id %d" sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3335) <https://reviews.apache.org/r/55246/#comment235448> Is it actually used by anything? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3367) <https://reviews.apache.org/r/55246/#comment235442> Do we ever execute with more than two transaction blocks? If not, do we really need to support the list in rm.executeTransactionBlocksWithRetry()? (We can simply provide a method with two TBs). sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 396) <https://reviews.apache.org/r/55246/#comment235440> This is strange - you are only using the last transaction block - is it intentional? - Alexander Kolbasov On Jan. 27, 2017, 7:41 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55246/ > ----------------------------------------------------------- > > (Updated Jan. 27, 2017, 7:41 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/DeltaTransactionBlock.java > PRE-CREATION > > 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 > 6428a0c8c231b1bf08bb3f48b2afbfb7c1d0dc7c > > 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 > 749c2ce8f89fe5960af5a4b48ff45a38091350f4 > > 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 TestSentryStore. > > > Thanks, > > Hao Hao > >
