----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19203/#review37125 -----------------------------------------------------------
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. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java <https://reviews.apache.org/r/19203/#comment68467> Why was this removed? We don't currently use it but I'd leave it there for the sake of symmetry. - Shreepadma Venugopalan 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 > >
