----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20803/#review42057 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75792> You mean getSentryRolesByGroupName? It is here on line 583. It also handled get all roles. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75793> Makes sense, let me change the comment in the next updated patch. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75794> Hmm might make sense, let me take a closer look at the duplication. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75795> If roles is empty, retrieval from ORM is not essential, and this block is skipped and we return an empty set. So we are good here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75796> Good catch, we should set this after the commit. Will change it in the next patch update. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75797> Will do sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75804> These are the utility functions for converting thrift object to model object and vise versa. Yes, technically we can move these to appropriate model class like MSentryRole class, but not sure if we will gain any thing by doing so. As these utilities are used only in SentryStore. By the way, apart from these conversion utilities, I also see some utilities which create the model object, which should really be a constructor. I will clean these up in my next patch update. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java <https://reviews.apache.org/r/20803/#comment75805> Will do sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java <https://reviews.apache.org/r/20803/#comment75806> Will do sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift <https://reviews.apache.org/r/20803/#comment75807> Yes, I agree. Let me file a separate jira for this and lets brainstorm alternatives. sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift <https://reviews.apache.org/r/20803/#comment75808> Ok - Sravya Tirukkovalur On May 1, 2014, 1:04 a.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20803/ > ----------------------------------------------------------- > > (Updated May 1, 2014, 1:04 a.m.) > > > Review request for sentry, Lenni Kuff and Prasad Mujumdar. > > > Bugs: SENTRY-184 > https://issues.apache.org/jira/browse/SENTRY-184 > > > Repository: sentry > > > Description > ------- > > This patch adds the following thrift API to query the list of privileges > granted for a given role: > * TListSentryPrivilegesResponse > list_sentry_privileges_by_role(1:TListSentryPrivilegesRequest request) > ** Allows to get granted privileges for a given role > > > It also fixes the issues with: > * TListSentryRolesResponse > list_sentry_roles_by_group(1:TListSentryRolesRequest request) > ** Looks up the roles for a given group. > ** Also can be used to list all roles in the system (when group_name = null) > > Getting rid of following API as we do not support mapping a role to multiple > roles. > * TListSentryRolesResponse > list_sentry_roles_by_role_name(1:TListSentryRolesRequest request) > > Outstanding issues: > > * We still need to decide on the semantics of "Show grant" and "show role > grant": > ** Right now, these meta data query operations can be performed only by the > admin user. But ideally we should allow even non admin users to query as long > as they are part of the group/role they are requesting info from. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 33c8d1af864282b2b56a54038573cfdcaa9a44f3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 7e73b2c7195c1f4561a30dab018bb5ba0d68f3ec > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 6c52fa421eaa5d036b522f827a9d0f8433676275 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > 677047fe359e346c2c3cd9f7bcb6597288d83cc5 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > dcaa246ea72e37c7aa7aed51f5c65b96bec61405 > > Diff: https://reviews.apache.org/r/20803/diff/ > > > Testing > ------- > > Added new tests. > > > Thanks, > > Sravya Tirukkovalur > >
