> On Nov. 13, 2015, 6:15 a.m., Li Li wrote: > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/DBPrivilegeCache.java, > > line 34 > > <https://reviews.apache.org/r/28800/diff/4/?file=1124779#file1124779line34> > > > > maybe better to add some class comments for the newly added classes?
I'll update it. > On Nov. 13, 2015, 6:15 a.m., Li Li wrote: > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/DBPrivilegeCache.java, > > line 71 > > <https://reviews.apache.org/r/28800/diff/4/?file=1124779#file1124779line71> > > > > maybe it is better to specify the exception type? The try clause throw the Exception, and don't specify the type, so currently catch Exception. > On Nov. 13, 2015, 6:15 a.m., Li Li wrote: > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java, > > line 36 > > <https://reviews.apache.org/r/28800/diff/4/?file=1124780#file1124780line36> > > > > make it final? I'll update it. > On Nov. 13, 2015, 6:15 a.m., Li Li wrote: > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java, > > line 35 > > <https://reviews.apache.org/r/28800/diff/4/?file=1124780#file1124780line35> > > > > make it final? > > make it public so that TestCacheProvider can use it in line 39? I'll update it. > On Nov. 13, 2015, 6:15 a.m., Li Li wrote: > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java, > > line 38 > > <https://reviews.apache.org/r/28800/diff/4/?file=1124780#file1124780line38> > > > > seems we can also remove this constructor method, since resourcePath is > > not used at all? I think this constructor here is for the compatibility, if the backend of the CacheProvider get the privileges from file, the resourcePath should be path of that file. > On Nov. 13, 2015, 6:15 a.m., Li Li wrote: > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java, > > line 42 > > <https://reviews.apache.org/r/28800/diff/4/?file=1124780#file1124780line42> > > > > Maybe it is better to specify the exception type, eg. > > ClassNotFoundException and etc. I'll update it. > On Nov. 13, 2015, 6:15 a.m., Li Li wrote: > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java, > > line 51 > > <https://reviews.apache.org/r/28800/diff/4/?file=1124780#file1124780line51> > > > > 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. The method inherit from the super class, and can't be delete, I'll add comments for it. > On Nov. 13, 2015, 6:15 a.m., Li Li wrote: > > 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/diff/4/?file=1124784#file1124784line121> > > > > 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. Thanks for catch that, I'll update it. > On Nov. 13, 2015, 6:15 a.m., Li Li wrote: > > 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/diff/4/?file=1124785#file1124785line2> > > > > 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? I think we don't need to add comments here, we can add the comments in the thrift defined file if necessary. We also can get the thrift version by the autogenerated comments. > On Nov. 13, 2015, 6:15 a.m., Li Li wrote: > > 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/diff/4/?file=1124786#file1124786line1266> > > > > I guess you intent to add a comment for this method? Thanks, I'll update it. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28800/#review106366 ----------------------------------------------------------- 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 > >
