> On June 27, 2018, 5 p.m., Arjun Mishra wrote: > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java > > Lines 311 (patched) > > <https://reviews.apache.org/r/67759/diff/2/?file=2046406#file2046406line311> > > > > Sergio, maybe this is the right opportunity to have a single method > > instead of one for roles and one for users and return something like > > Map<TSentryPrincipal, Set<TSentryPrivileges>> ? > > Sergio Pena wrote: > I thought about that early, but the intention of this API is that you > could get either all roles and privileges or all users and privileges. If we > do return all users and roles, then the caller of this API would need to > separate roles and users which would take O(n) time where n is the number of > roles + users. > > For instance: > Map<TSentryPrincipal, Set<TSentryPrivileges>> privileges = > client.list_all_privileges(); > > Map<String, Set<TSentryPrivileges>> allRoles, allUsers; > for (TSentryPrincipal princ : privileges.keySet()) { > if (princ.getType() == SentryPrincipal.ROLE) > allRoles.put(princ.getName(), privileges.get(princ)); > else if (princ.getType() == SentryPrincipal.USER) > allUsers.put(princ.getName(), privileges.get(princ)); > else > ;// principal not supported > } > > With the above code, now the caller can check if a specific role or a > specific user has privileges, or display only user privileges or only role > privileges. > That's why I didn't go with the above solution. If I go to the current > one, then the caller would not need to split users vs roles, and it will be > able to check privileges in O(1). > > Another idea is to add in the request to return only user privileges or > only role privileges, but how can the caller made sure the returned > principals are for user or for roles? They would need to check if the > princ.geType() is equals to USER or ROLE everytime just to make sure there > were no errors. > > So, I prefered to sacrify two API calls instead of 1 to let the caller > have a simple data type with either roles or users.
Sergio, there is another approach can address both Arjun's request and your concern. Map<type, Map<TSentryPrincipal, Set<TSentryPrivileges>>>. Tye can be user or role. Under user, all entries are about user; under role, all entries are about role. So it does not take O(n). Also, it can save round trip time. Caller can get all privileges for both user and role in one call, instead of two calls, one for user and another for role. It is particularly important when caller is far away from the sentry server - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67759/#review205452 ----------------------------------------------------------- On June 27, 2018, 7:33 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67759/ > ----------------------------------------------------------- > > (Updated June 27, 2018, 7:33 p.m.) > > > Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Na Li. > > > Bugs: sentry-2284 > https://issues.apache.org/jira/browse/sentry-2284 > > > Repository: sentry > > > Description > ------- > > Add two API methods to the Sentry client: > - listAllRolesPrivileges > - listAllUsersPrivileges > > Both methods return a map object of the form: > - [roleName, set<privileges>] > - [userName, set<privileges>] > > Unit tests, thrift API code and Client methods are provided. > > > Diffs > ----- > > sentry-dist/src/license/THIRD-PARTY.properties > b39e1b6ca7eba8c6a7695a4238104af7cd50da32 > > sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java > d456d7ff64a8146473ff14a65f06e7cb664939b7 > > sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesRequest.java > PRE-CREATION > > sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesResponse.java > PRE-CREATION > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClient.java > dc1d67ba16c9c7acf76eb3318864ba0606e2aa5a > > sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 5f76cb0aa3bd17d6aca4adc52ba59ded5ec0b900 > > sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift > 56aedcb5267495e610e3dace1f38e708d68ffe84 > > sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyServiceClientDefaultImpl.java > 1666e326462b771674930e52e3790ca92f467778 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > 39271500df179e0ddf8ce4f1589eaaa8137afa25 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > d8ab1fc90255c6ed42c00e3d3aa6103e47a40b29 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java > f61ae575c99474a76886d3c7a74765fdb067acd6 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java > de4e0012d521cf402f4773a04b673f8056a5337c > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 52ce72cebc8b69f08988b05ba54e2bbedd201c1a > > > Diff: https://reviews.apache.org/r/67759/diff/3/ > > > Testing > ------- > > > Thanks, > > Sergio Pena > >
