----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28800/#review106366 -----------------------------------------------------------
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/DBPrivilegeCache.java (line 34) <https://reviews.apache.org/r/28800/#comment165111> maybe better to add some class comments for the newly added classes? sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/DBPrivilegeCache.java (line 71) <https://reviews.apache.org/r/28800/#comment165103> maybe it is better to specify the exception type? sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java (line 35) <https://reviews.apache.org/r/28800/#comment165104> make it final? make it public so that TestCacheProvider can use it in line 39? sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java (line 36) <https://reviews.apache.org/r/28800/#comment165105> make it final? sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java (line 38) <https://reviews.apache.org/r/28800/#comment165101> seems we can also remove this constructor method, since resourcePath is not used at all? sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java (line 42) <https://reviews.apache.org/r/28800/#comment165102> Maybe it is better to specify the exception type, eg. ClassNotFoundException and etc. sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java (line 51) <https://reviews.apache.org/r/28800/#comment165099> should we update the method comment since currently it is only initailized once, or just deleted this method? Besides, it is better to add an empty line between two methods. sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryPrivilegesForCachedRequest.java (line 121) <https://reviews.apache.org/r/28800/#comment165100> it is better to remove the trailing space here and line 123, 124, 126, 521, 529, 547, 556, and also in SentryPolicyService.java and TListSentryPrivilegesForCachedResponse.java. sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryPrivilegesForCachedResponse.java (line 2) <https://reviews.apache.org/r/28800/#comment165109> since this file and the other java files under thrift dir are autogenrated by Thrift compiler, should we add some comments for those manual changes? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1266) <https://reviews.apache.org/r/28800/#comment165110> I guess you intent to add a comment for this method? - Li Li 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 > >
