You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Lenni Kuff <ls...@cloudera.com> on 2015/01/09 00:31:21 UTC
Re: Review Request 29040: SENTRY-567: InMemory implementation of
SentryStore
-----------------------------------------------------------
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
>
>