> On May 6, 2014, 3:35 p.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 706 > > <https://reviews.apache.org/r/20803/diff/4/?file=574981#file574981line706> > > > > I would remove this function since it only saves you a few characters > > and makes it harder to understand the time units when reading the code.
Fixed. > On May 6, 2014, 3:35 p.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 711 > > <https://reviews.apache.org/r/20803/diff/4/?file=574981#file574981line711> > > > > Explain what "normalization" means in this context. Trimming and toLowerCase. By the way, this comment was there before this patch. > On May 6, 2014, 3:35 p.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java, > > line 322 > > <https://reviews.apache.org/r/20803/diff/4/?file=574982#file574982line322> > > > > Should this return a TSentryPrivilege? This API is not part of this patch. This API is specifically used by the Sentry Provider, where the provider needs a set of privileges in the form of strings. But now that we have a way to list roles and privileges, this handling should probably go inside hive binding code. Will investigate this as a follow on. > On May 6, 2014, 3:35 p.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java, > > line 175 > > <https://reviews.apache.org/r/20803/diff/4/?file=574982#file574982line175> > > > > You shouldn't need to include the getMessage() in the string since you > > are also passing the "cause" of the exception. > > > > You can consider changing these to: > > new SentryUserException("Thrift exception occurred ", e); > > > > Feel free to address this in a follow on change. Changed. - Sravya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20803/#review42291 ----------------------------------------------------------- On May 6, 2014, 10:24 p.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20803/ > ----------------------------------------------------------- > > (Updated May 6, 2014, 10:24 p.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/SimpleDBProviderBackend.java > b068aca22dbcc930a25b014a3140659fb55537de > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > 1dfc0cf99cfe1b740e0b20a0d7f06ab06d8dfc1d > > 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 > >
