----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68822/#review208969 -----------------------------------------------------------
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java Lines 66 (patched) <https://reviews.apache.org/r/68822/#comment293235> Can you change the function from "list_sentry_privileges_for_user" to "list_sentry_privileges_by_user_and_itsgroups" to make it clear that it includes privileges directly assigned to this user and privileges through its groups? You don't have to use "list_sentry_privileges_by_user_and_itsgroups" exactly, but use some name similar to it. sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift Lines 416 (patched) <https://reviews.apache.org/r/68822/#comment293236> "and the group" to "and the groups" since a user can belong to multiple groups. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java Lines 113 (patched) <https://reviews.apache.org/r/68822/#comment293237> change the function name from "list-privileges-for-user" to the one you will use in sentry_policy_service.thrift sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 57 (patched) <https://reviews.apache.org/r/68822/#comment293238> It is better to add the new function in SentryStoreInterface instead of SentryStore as this class deals with the interface, not its implementation sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 915 (patched) <https://reviews.apache.org/r/68822/#comment293240> you have got groups in line 908 if the requestor is the same as principlaName. It should improve performance to reuse it under this situation sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java Line 227 (original), 229 (patched) <https://reviews.apache.org/r/68822/#comment293242> This is semi-public API. It is safer to add function than change the returned type of existing function. Inside implementation, you can let one function call another one to re-use code. sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java Lines 26 (patched) <https://reviews.apache.org/r/68822/#comment293243> If you only add new function in SentryStoreInterface, you can make it part of the TestSentryStore if those testing cases don't exsit. - Na Li On Sept. 24, 2018, 9:31 p.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68822/ > ----------------------------------------------------------- > > (Updated Sept. 24, 2018, 9:31 p.m.) > > > Review request for sentry, Na Li and Sergio Pena. > > > Repository: sentry > > > Description > ------- > > This commit adds a new thrift API list_sentry_privileges_for_user to List > sentry privileges granted to the given user and the group the user associated > with, filterted based on authorization hierarchy if present. > Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider. > > > Diffs > ----- > > > sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java > 0cbd8ab0a624d4c09aead4097f72762e12d1d21b > > sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift > 2e79e5646ae9102d8c0c28da4260a539254fcd15 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java > 236a07bdf5191cdc0f167f20a406b721b3dc506d > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > 3a9623b46f7c4335db18113574170f761da9a4ca > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 1eda41b2b6bd940a404cc1ba09a861fe783ead04 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java > 0b4f4aa24bd3002c50bf4d80a6fa361f66052973 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java > PRE-CREATION > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java > 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4 > > > Diff: https://reviews.apache.org/r/68822/diff/2/ > > > Testing > ------- > > Unit test. > > > Thanks, > > Hao Hao > >
