> On March 13, 2014, 10:52 p.m., Shreepadma Venugopalan wrote: > > LGTM mostly. I've a high level comment - we're bulk fetching the privileges > > for one or more groups which is great. But I'm not convinced we should > > return just a set of strings. There's other information in the Privilege > > and Role object that's of interest we are missing out. In the interest of > > time though, I'm OK with the API as is, but we should add bulk fetch APIs > > that a) return the set of valid roles as Role object for a given group, b) > > return the set of valid privileges for as Privilege object for a given > > group.
Agreed 100%. This API was created specifically for the provider use case. I will add a comment to that effect. > On March 13, 2014, 10:52 p.m., Shreepadma Venugopalan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, > > line 156 > > <https://reviews.apache.org/r/19203/diff/1/?file=518984#file518984line156> > > > > Why was this removed? We don't currently use it but I'd leave it there > > for the sake of symmetry. That method didn't work because the roles set is modified while being iterated. Since it wasn't being used and is broken I removed it. We always lookup the role before performing a delete so I think the removeRole() method covers our needs. - Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19203/#review37125 ----------------------------------------------------------- On March 13, 2014, 9:14 p.m., Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19203/ > ----------------------------------------------------------- > > (Updated March 13, 2014, 9:14 p.m.) > > > Review request for sentry and Shreepadma Venugopalan. > > > Bugs: SENTRY-142 > https://issues.apache.org/jira/browse/SENTRY-142 > > > Repository: sentry > > > Description > ------- > > Creates a db-backend for the the provider. Changes: > > 1) I had to introduce a close() method to the provider so that the db-backend > can close it's connection to the service. > 2) I made SentryPolicyServiceClient more user friendly. > 3) The new API list_sentry_privileges_for_provider does all filter on the > server side and puts the privileges in the "authorizable" format as I wanted > to create as little object overhead as possible since I could see users > having thousands of privileges. > 4) Patch looks bigger than it is due the renames below. > > rename > sentry-provider/{sentry-provider-file/src/main/java/org/apache/sentry/provider/file > => > sentry-provider-common/src/main/java/org/apache/sentry/provider/common}/HadoopGroupMappingService.java > (97%) > rename > sentry-provider/{sentry-provider-file/src/main/java/org/apache/sentry/provider/file > => > sentry-provider-common/src/main/java/org/apache/sentry/provider/common}/HadoopGroupResourceAuthorizationProvider.java > (94%) > rename > sentry-provider/{sentry-provider-file/src/main/java/org/apache/sentry/provider/file > => > sentry-provider-common/src/main/java/org/apache/sentry/provider/common}/ResourceAuthorizationProvider.java > (95%) > rename > sentry-provider/{sentry-provider-file/src/test/java/org/apache/sentry/provider/file > => > sentry-provider-common/src/test/java/org/apache/sentry/provider/common}/TestGetGroupMapping.java > (93%) > rename > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/{service/persistent > => }/SentryAlreadyExistsException.java (94%) > rename > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/{service/persistent > => }/SentryInvalidInputException.java (94%) > rename > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/{service/persistent > => }/SentryNoSuchObjectException.java (94%) > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingPreExecHook.java > bed7917 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java > 65854c3 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java > c4f12b5 > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java > 70983c4 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/ActiveRoleSet.java > c1f1f66 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java > 512e28e > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java > e67daf4 > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestResourceAuthorizationProviderGeneralCases.java > 469be14 > > sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java > 728e356 > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/TestSearchAuthorizationProviderGeneralCases.java > 6f36243 > sentry-provider/sentry-provider-common/pom.xml 1e9dc1b > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationProvider.java > cd6f8a1 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/HadoopGroupMappingService.java > PRE-CREATION > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/HadoopGroupResourceAuthorizationProvider.java > PRE-CREATION > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/NoAuthorizationProvider.java > ed32224 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java > 6d6da25 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderConstants.java > PRE-CREATION > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > PRE-CREATION > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/file/HadoopGroupResourceAuthorizationProvider.java > PRE-CREATION > > sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java > PRE-CREATION > sentry-provider/sentry-provider-db/pom.xml aa511c8 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryAlreadyExistsException.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryInvalidInputException.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryNoSuchObjectException.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > 7215435 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > 16be80b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryAlreadyExistsException.java > 965e64c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryInvalidInputException.java > 6ac9942 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryNoSuchObjectException.java > a976880 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f1e502a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > a4487ee > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 3fe47dc > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 253f88e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Status.java > 1686780 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > b3f7d6e > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > be3d078 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > d073d8b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java > db76aa8 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/HadoopGroupMappingService.java > f2bb39c > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/HadoopGroupResourceAuthorizationProvider.java > b2e4196 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/LocalGroupResourceAuthorizationProvider.java > e8293f6 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFileConstants.java > d28cde2 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/ResourceAuthorizationProvider.java > 448d7c1 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java > 89a2d31 > > sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestGetGroupMapping.java > d3127d7 > > Diff: https://reviews.apache.org/r/19203/diff/ > > > Testing > ------- > > New unit tests and they pass. > > > Thanks, > > Brock Noland > >
