-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28800/#review107488
-----------------------------------------------------------
Thanks for putting in the effort for this patch! I'm a little confused about
what this is doing and how it is solving the problem. Is the problem that when
you do a SHOW TABLES that we make a RPC to Sentry to check the same privielges
for each one of the tables? My comments below assume that's the case:
1) Why are we adding any new thrift interfaces here? It seems like this should
be contained within the HMS plugin?
2) Do we need to have a cache that expires? That seems to add complexity here.
3) What does it even mean to have an RPC for a cached privilege set?
I would expect a solution that looked like (using psedo code):
List<Privilege> getPrivilegesForUse(String username);
List<String> filterTables() {
List<Privilege> userPrivileges = getPrivilegesForUser(username);
// Note, we create a new cache each time filterTables() is called.
PrivilegeCache cache = new PrivilegeCache(userPrivileges);
AuthProvider provider = new AuthProvider(cache);
for table in getAllTables() {
if (privider.hasAccess(table) ) {
// include this table;
}
}
}
It appears that your solution is trying to maintain a global cache, which I
don't think is necessary. I also don't think that any thrift changes should be
required to make this work.
Does this make sense? Am I missing something?
- Lenni Kuff
On Nov. 13, 2015, 4:10 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28800/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2015, 4:10 a.m.)
>
>
> Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Currently, when get the metadata from hive, eg, "show tables", "show
> databases". Sentry will filter the result and output the authorized entities.
> There will be many RPC calls when filtering the result. The related code is
> in HiveAuthzBinding, for example, in filterShowTables:
>
> ......
> for (String tableName : queryResult) {
> ......
> hiveAuthzBinding.authorize(operation, tableMetaDataPrivilege, subject,
> inputHierarchy,
> outputHierarchy, providedPrivileges);
> ......
> }
> ......
>
> hiveAuthzBinding.authorize will get the privileges from sentry service, if
> there are many tables in the hive, the filtering process will spend much
> time. Considering sentry also need to filter the column, HiveAuthzBinding
> should be improved to reduce the number of rpc calls when doing the filter.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-cache/pom.xml c67f094
>
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/CachedPrivilegeWrap.java
> PRE-CREATION
>
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/DBPrivilegeCache.java
> PRE-CREATION
>
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
> 4b98447
>
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java
> a7566e7
>
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestCacheProvider.java
> e5b29b8
>
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/SentryPolicyService.java
> 0c24449
>
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryPrivilegesForCachedRequest.java
> PRE-CREATION
>
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryPrivilegesForCachedResponse.java
> PRE-CREATION
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> 8c9401c
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java
> cbc0aaf
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java
> 74f379a
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
> 4f8c834
>
> sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
> 40889e8
>
> Diff: https://reviews.apache.org/r/28800/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
