----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28893/#review64883 -----------------------------------------------------------
Some comments from an initial pass over. Overall it looks really cool. It would be good clarified the object model and persistence behavior someplace in the code. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DbSentryStore.java <https://reviews.apache.org/r/28893/#comment107755> Fix? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/FileLoggingSentryStore.java <https://reviews.apache.org/r/28893/#comment107689> Add class comment that explains the persistence behavior. Also the current class name sounds kind of like this is a SentryStore that logs more debug information, consider renaming. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/FileLoggingSentryStore.java <https://reviews.apache.org/r/28893/#comment107772> The current implementation seems to treat these logs as "unbounded". You will need to always store the complete history of all commits and then replay that when you start back up. This is probably okay for now, but it doesn't scale well. Add TODO or comment. Also, it would be nice to have metrics on things like the time it took to recover, etc. Add them or add a TODO. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/FileLoggingSentryStore.java <https://reviews.apache.org/r/28893/#comment107771> Why is this Atomic? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/FileLoggingSentryStore.java <https://reviews.apache.org/r/28893/#comment107693> Preconditions.checkState(lastSeqNum < seqId). nit: consitent use of seqNum or seqId but not both. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/FileLoggingSentryStore.java <https://reviews.apache.org/r/28893/#comment107741> This is just the initial prototype, but it's probably good to explictly call out the log format. We also might want to encode some version information in it early on to handle the case where the format changes. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/FileLoggingSentryStore.java <https://reviews.apache.org/r/28893/#comment107742> How will we detect the scenario where the log is corrupted? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/FileLoggingSentryStore.java <https://reviews.apache.org/r/28893/#comment107699> You are committing the record when it does not have an associated item in the set of reverted commits. What if your app started an operation, but crashed before writing out the revereted commits? Seems a bit dangerous, no? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/28893/#comment107744> nit: define in hex to make this more clear. Also, does ALL belong in this enum? It seems like it should be defined dynamically by OR'ing all the privileges. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/28893/#comment107746> clarify why sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/28893/#comment107770> remove sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/28893/#comment107748> consider a more descriptive name. What is this checking? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/28893/#comment107773> getLeafAuthorizeable? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/28893/#comment107774> what does isParentOk mean? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/28893/#comment107775> The code in the if and else blocks looks nearly identical, consolidate? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/28893/#comment107777> We should think about how we can start adding some abstraction on top of this so we don't have the db-model specific code wrapped in here. Not needed in this patch, but something to consider. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/28893/#comment107778> There are lots of maps of maps around. Do you think it makes sense to create a data structure that hides these details? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PersistentSentryStore.java <https://reviews.apache.org/r/28893/#comment107781> Who uses this? Is it used by the db-backed store? The FileLogger store? Would be good to make this clear. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/ThreadSafeSentryStore.java <https://reviews.apache.org/r/28893/#comment107752> It would be nice if there was a bit more granular locking. For the time being should we just make everything synchronized? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java <https://reviews.apache.org/r/28893/#comment107753> ? sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift <https://reviews.apache.org/r/28893/#comment107754> "a Privilege in transport". Can you clarify? Also, if this is a privilege (singular) it's strange we include a set of privileges here. - Lenni Kuff On Dec. 10, 2014, 8:01 a.m., Arun Suresh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28893/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2014, 8:01 a.m.) > > > Review request for sentry, Lenni Kuff, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > The basic motivations for this JIRA is > > 1) Make the Sentry Store pluggable > 2) Provide an implementation of SentryStore that in not dependent on JDO / > Datanucleus or any external data layer. > 3) Separate out the core DataStructures and Persistence logic > > Current status : > 1) It includes a new InMemSentryStore and a ThreadSafeInMemSentryStore > 2) It also includes a persistent FileLoggingSentryStore : This writes out > sentry store operations to a log file and on restart, reads the log file and > populates a backing InMemSentryStore > 3) The tests in sentry-hive-tests can be run against the new store > impementationss : > * add env variable USE_IN_MEM=true to use the in memory store > * add env variable USE_FILE_STORE=true to use the FileLoggingSentryStore > with a backing in memory store > 4) All TestSentrySeviceIntegration tests pass > 5) There is a senparate TestInMemSentryStore that runs most of the same tests > as TestSentryStore > > TODO: > 1) Add support for log compaction > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > f1e792d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > 1c68a0f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DbSentryStore.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/FileLoggingSentryStore.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PersistentSentryStore.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f98e853 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreFactory.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/StoreUtils.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/ThreadSafeSentryStore.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java > 55bec0b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 29e3131 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > 993ea46 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestFileLoggingSentryStore.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestInMemSentryStore.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 8fbe3f4 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java > 922cbc2 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java > 0add58b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > 4a6cac9 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java > be14afd > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java > 4a475ba > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbDDLAuditLog.java > 2cecdfd > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java > acb789f > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > f8cc1d0 > > Diff: https://reviews.apache.org/r/28893/diff/ > > > Testing > ------- > > > Thanks, > > Arun Suresh > >
