> On Feb. 3, 2017, 10:59 p.m., Vadim Spector wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java, > > line 295 > > <https://reviews.apache.org/r/55246/diff/9/?file=1622992#file1622992line295> > > > > general question for all onAlterSentryRole...() callbacks: sequence > > number update permSeqNum.incrementAndGet() and > > permsUpdater.handleUpdateNotification() calls are not synchronized > > together. It means, that permsUpdater.handleUpdateNotification(update) can > > take "update" object with the sequence numbers out-of-order. Can it become > > a problem?
It seems that it can be, looking at UpdateForwarder.java: handleUpdateNotification() has "editMissed" boolean detecting out-of-sequence numbers and triggering the whole "Retrieve full update from External Source" logic. It could be performance overhead at least, do you think? Could getAllUpdatesFrom() return incomplete list of updates to NN because of that? I'm not sure here, without more thorough analysis. But it seems to me, that no harm would be done if all methods in SentryPlugin that both update the update sequence number and call permsUpdater.handleUpdateNotification() could do both operations synchronized together. Both operations are cheap, so it should not harm the performance - What do you think? - Vadim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55246/#review164182 ----------------------------------------------------------- On Feb. 3, 2017, 5:47 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55246/ > ----------------------------------------------------------- > > (Updated Feb. 3, 2017, 5:47 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 > 1f05ba9cb6483251821f2965808dfd0711e70c63 > > 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/service/thrift/TestHMSFollower.java > d601b1e107ab3c3a4d9cc5e3038a11547182c5c9 > > Diff: https://reviews.apache.org/r/55246/diff/ > > > Testing > ------- > > New unit tests added in TestSentryStore. > > > Thanks, > > Hao Hao > >
