----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28800/#review101398 -----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java (line 668) <https://reviews.apache.org/r/28800/#comment158808> Can you add a brief comment on this method and explain the return value? Maybe rename to getActivePrivilegesForUser()? sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java (line 67) <https://reviews.apache.org/r/28800/#comment158810> This is kind of a strange API to me. The caller of this class shouldn't really care about what the privileges are and why would they should rely on Sentry as the source of truth. I don't know if we want to add this to our main AuthorizationProvider class. One thing to also look at is the PrivilegeCache / SimpleCacheProviderBackend classes. They are not quite what you are looking for, but do allows you to inject privileges. In your case you could implement a version of that class that just calls getPrivilegesForUser()... sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java (line 149) <https://reviews.apache.org/r/28800/#comment158809> I feel like the previous API was much more structured. You have just encoded this structure into strings, which is going to be harder to debug. I also don't understand why you would call getPrivileges and pass set of privileges. Think a bit more about the naming and see if you can find a way to avoid the convertion to strings . - Lenni Kuff On Dec. 8, 2014, 12:14 p.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28800/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2014, 12:14 p.m.) > > > Review request for sentry. > > > 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-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > 97ef3b8 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingPreExecHook.java > 813200a > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java > b4b69e1 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java > e7f96c1 > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java > d41f6cf > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java > a88d2f8 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/NoAuthorizationProvider.java > a814527 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > 6449405 > > Diff: https://reviews.apache.org/r/28800/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
