----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29040/#review68411 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112630> I think we also need a privilege "ALTER", so that we have permission to rename. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112620> could you describe the meaning of the return value of addTo and removeFrom? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112616> agree with Lenni. But AccessConstants.ACTION_ALL is "ALL", and AccessConstants.ALL is "*". Fix a typo, change ACTION_ALL to ALL. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112614> Why we define GrantOption? Reuse TSentryGrantOption is not enough? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112627> rename AuthType to AuthScope maybe better? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112629> Could you comment what rootAuthrizatbles doing for? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112628> When roleToGroups.containsKey(roleName) but !roleToAuthorizable.containsKey(roleName)? I think roleToAuthorizable.containsKey(roleName) is enough. hmm..how about define a set<String> roles, doseRoleExists() and getRoleCount() from this set? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112613> use "equals" maybe better than "==" sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112631> I think this is a workaround method to handle partial revoke. If I grant privilege INSERT, SELECT, DROP and CREATE, then I get privilege by roleName, it will return 2 privileges, right? But I think 4 is right. We should make partial revoke just support privilege ALL, INSERT and SELECT. It meanings after revoking ALL privilege, roletoAuthorizable just retain SELECT privilege. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112608> can we use switch ... case ? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/InMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112609> why store time as 0? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestInMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112632> Could you add test cases from TestSentryStore.testDropTableWithMultiAction() and testRenameTableWithMultiAction? Another suggesion: we don't have to add same test cases for every sentryStroe, could we use same in base class? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestInMemSentryStore.java <https://reviews.apache.org/r/29040/#comment112633> please remove these deprecated code. Hi Arun, I have some comments about this in memory SentryStore, please feel free to fix them. - Xiaomeng Huang On 十二月 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 十二月 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 > >
