----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29040/#review66929 -----------------------------------------------------------
Dropped some comments and ideas. It would be good to have a few more comments in the code describing the purpose of the various classes and how callers should use various methods, but overall looking good. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment111313> Can this just be a Boolean? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment111001> mention details about privilege code is. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment111002> Should we have an "ALL" privilege type or should we have a constant that is a union of 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/29040/#comment111316> Should these method that manipulate the privilege codes be static? You are passing a Priv object in each time. Also, should "includedIn" be named "implies"? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment110651> Maybe just do: if (AccessConstants.ACTION_ALL.equals(priv)) return ALL; try { return Privilege.valueOf(priv.toUpperCase()); } catch (...) { return NONE; } sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment110657> Maybe rename privilege -> privilegeCode here and elsewhere? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment110652> Do we not have this enum anywhere else? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment110658> commment on what the key/values are for all maps. Also, the maps should probably be named something like blahToBlah. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment111314> I think "name" is case insensitive, so you should be sure to handle that here and elsewhere and mention someplace that it we expect case in-sensitive keys. Also add the public/private scope qualifiers to all of these methods. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment111317> remove sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment111004> rename to explain what this is checking. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment111318> Would it be useful to create an AuthorizeableHierarchy data structure that internally uses a HashMap? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment111005> I think this needs a comment on what is is doing and what the parameters mean. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment111009> resultMap -> resultSet sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment111320> Also consider how we can reduce duplicate code between the SentryStore implementations. For example, each implementation will need to do this same doesRoleExist/throw SentryUserException... and it's easy to get it wrong across implementations. It would be nice if the SentryStore provide an API for accessing all metadata: getRole(), getRolePrivilege(), etc.. And the execution of the different operations was decoupled (ie - not part of SentryStore at all). Does that make sense? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment111319> Nice! sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment110653> Seems like it should be fixed sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreFactory.java <https://reviews.apache.org/r/29040/#comment111010> comments on class and createSentryStore sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreFactory.java <https://reviews.apache.org/r/29040/#comment111011> Probably should keep the default be "db" sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreFactory.java <https://reviews.apache.org/r/29040/#comment111013> Probably should keep the default be "db" sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestInMemSentryStore.java <https://reviews.apache.org/r/29040/#comment110655> Are the tests for each store implementation going to be pretty much the same? Can we take the common test cases and put them in a single test suite that just uses different store implementations? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestInMemSentryStore.java <https://reviews.apache.org/r/29040/#comment110654> Anything special with the random file path? - Lenni Kuff On Dec. 18, 2014, 8:01 a.m., Arun Suresh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29040/ > ----------------------------------------------------------- > > (Updated Dec. 18, 2014, 8:01 a.m.) > > > Review request for sentry, bc Wong, Lenni Kuff, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > This patch includes a purely inmemory implementation of sentry store and > associated testcases > More decription on the JIRA : issues.apache.org/jira/browse/SENTRY-567 > > > Diffs > ----- > > > 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/SentryStoreFactory.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 29e3131 > > 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/TestStoreSnapshot.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/29040/diff/ > > > Testing > ------- > > > Thanks, > > Arun Suresh > >
