You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Gregory Chanan <gc...@cloudera.com> on 2013/10/02 00:47:48 UTC
Re: Review Request 14344: SENTRY-17: Separate sentry-provider to hive
specific and non-specific packages
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14344/
-----------------------------------------------------------
(Updated Oct. 1, 2013, 10:47 p.m.)
Review request for sentry and Brock Noland.
Changes
-------
New diff based on previous discussion. Let me know if you think this is ready to commit.
>From the previous comment:
1) I'll file a JIRA capturing these ideas and assign myself
I filed SENTRY-27
2) I'll rename the sentry-provider-hive module to sentry-provider-policy-db to avoid hive-specific naming for now and to indicate it includes both provider and policy code for now.
This is done and all the Hive specific classes in there have been renamed to DB. The java package names are still "provider.db" rather than "provider.policy.db" because the non-policy stuff is going to stay and renaming the package back and forth seemed like a pain.
3) I'll rename the sentry-provider-file package to sentry-provider-backend-file (optional? Do you think this is good?)
I decided against this for now; I'm not sure what provider means or how it's different than backend right now. Having both seems superfluous, but we can revisit.
NOTE: unfortunately the diff didn't come out as nicely as before with the renames (in the previous patch I used "git -M"). It guessed wrong on the renames when I tried this time, so you'll see entire files deleted and recreated in the diff.
Bugs: SENTRY-17
https://issues.apache.org/jira/browse/SENTRY-17
Repository: sentry
Description
-------
Separated out the hive specific stuff in sentry-provider-file into a sentry-provider-hive package. This seemed like a reasonable first step; later if we want to combine some hive and other-db service (like impala), we can change the name to sentry-provider-db. I'm not sure how we would separate things out if we had another equivalent of sentry-provider-file (what would that even look like?), but we can deal with that later as well, I think.
Most of this is fairly straightforward; if there is some hive specific class in sentry-provider-file, I made it an interface and moved the implementing class to sentry-provider-hive. The exceptions/issues are as follows:
1) SimplePolicyEngine: I separated this out into two parts, the parser (SimplePolicyParser) and the hive-specific implementation (SimpleHivePolicyEngine) that uses the parser. This is a "prefer composition over inheritance" thing, because if the SimplePolicyEngine is a base class it ends up calling getPermissions on the derived class from its constructor, which could cause problems with uninitialized derived-class members.
2) The Roles interface is database/hive specific today, because I'm not sure what another service would want as an interface yet. We can revisit at some point.
NOTE: this patch uses SENTRY-4 as a parent patch, so assumes that has already been committed, but it will either apply correctly or with very minor fixups without that patch.
Diffs (updated)
-----
pom.xml c4aaf07
sentry-binding/sentry-binding-hive/pom.xml 2d4369e
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java fde3181
sentry-dist/pom.xml 0f98d41
sentry-dist/src/main/assembly/src.xml bf41707
sentry-provider/pom.xml b4e7689
sentry-provider/sentry-provider-file/pom.xml d09ad9f
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/AbstractRoleValidator.java 35889e4
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/Authorizables.java 4062473
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/DatabaseMustMatch.java ef6486b
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/DatabaseRequiredInRole.java fd0f2c1
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/HadoopGroupResourceAuthorizationProvider.java f99ae8c
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/LocalGroupResourceAuthorizationProvider.java ef595c8
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PermissionFactory.java PRE-CREATION
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/ResourceAuthorizationProvider.java 60282e6
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/Roles.java 924c2cc
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/RolesFactory.java PRE-CREATION
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/ServerNameMustMatch.java 1d2a8c6
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/ServersAllIsInvalid.java 8ee1c43
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimplePolicyEngine.java 0d4c858
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimplePolicyParser.java PRE-CREATION
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/WildcardPermission.java 23c845d
sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestAuthorizables.java f81b574
sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestDatabaseRequiredInRole.java fc35043
sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestPolicyParsingNegative.java 7285806
sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestResourceAuthorizationProviderGeneralCases.java a8a946d
sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestResourceAuthorizationProviderSpecialCases.java 14e2ff5
sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestSimplePolicyEngineDFS.java 656a0fa
sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestSimplePolicyEngineLocalFS.java 73cd673
sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestWildcardPermission.java 156aa64
sentry-provider/sentry-provider-file/src/test/resources/test-authz-provider-other-group.ini cd3695c
sentry-provider/sentry-provider-file/src/test/resources/test-authz-provider.ini 2d00699
sentry-provider/sentry-provider-policy-db/pom.xml PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/AbstractDBRoleValidator.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/DBAuthorizables.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/DBRoles.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/DBWildcardPermission.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/DatabaseMustMatch.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/DatabaseRequiredInRole.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/HadoopGroupResourceAuthorizationProvider.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/LocalGroupResourceAuthorizationProvider.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/ServerNameMustMatch.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/ServersAllIsInvalid.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/SimpleDBPolicyEngine.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestDBAuthorizables.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestDBWildcardPermission.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestDatabaseRequiredInRole.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestPolicyParsingNegative.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestResourceAuthorizationProviderGeneralCases.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestResourceAuthorizationProviderSpecialCases.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestSimpleDBPolicyEngineDFS.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestSimpleDBPolicyEngineLocalFS.java PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/test/resources/log4j.properties PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/test/resources/test-authz-provider-other-group.ini PRE-CREATION
sentry-provider/sentry-provider-policy-db/src/test/resources/test-authz-provider.ini PRE-CREATION
sentry-tests/sentry-tests-hive/pom.xml 27b45c0
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPerDBConfiguration.java 17f4de1
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 288a7b3
Diff: https://reviews.apache.org/r/14344/diff/
Testing
-------
Ran "mvn test -Pdownload-hadoop" successfully.
Thanks,
Gregory Chanan
Re: Review Request 14344: SENTRY-17: Separate sentry-provider to hive
specific and non-specific packages
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14344/#review26609
-----------------------------------------------------------
Ship it!
Ship It!
- Brock Noland
On Oct. 1, 2013, 10:47 p.m., Gregory Chanan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14344/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2013, 10:47 p.m.)
>
>
> Review request for sentry and Brock Noland.
>
>
> Bugs: SENTRY-17
> https://issues.apache.org/jira/browse/SENTRY-17
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Separated out the hive specific stuff in sentry-provider-file into a sentry-provider-hive package. This seemed like a reasonable first step; later if we want to combine some hive and other-db service (like impala), we can change the name to sentry-provider-db. I'm not sure how we would separate things out if we had another equivalent of sentry-provider-file (what would that even look like?), but we can deal with that later as well, I think.
>
> Most of this is fairly straightforward; if there is some hive specific class in sentry-provider-file, I made it an interface and moved the implementing class to sentry-provider-hive. The exceptions/issues are as follows:
> 1) SimplePolicyEngine: I separated this out into two parts, the parser (SimplePolicyParser) and the hive-specific implementation (SimpleHivePolicyEngine) that uses the parser. This is a "prefer composition over inheritance" thing, because if the SimplePolicyEngine is a base class it ends up calling getPermissions on the derived class from its constructor, which could cause problems with uninitialized derived-class members.
> 2) The Roles interface is database/hive specific today, because I'm not sure what another service would want as an interface yet. We can revisit at some point.
>
> NOTE: this patch uses SENTRY-4 as a parent patch, so assumes that has already been committed, but it will either apply correctly or with very minor fixups without that patch.
>
>
> Diffs
> -----
>
> pom.xml c4aaf07
> sentry-binding/sentry-binding-hive/pom.xml 2d4369e
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java fde3181
> sentry-dist/pom.xml 0f98d41
> sentry-dist/src/main/assembly/src.xml bf41707
> sentry-provider/pom.xml b4e7689
> sentry-provider/sentry-provider-file/pom.xml d09ad9f
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/AbstractRoleValidator.java 35889e4
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/Authorizables.java 4062473
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/DatabaseMustMatch.java ef6486b
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/DatabaseRequiredInRole.java fd0f2c1
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/HadoopGroupResourceAuthorizationProvider.java f99ae8c
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/LocalGroupResourceAuthorizationProvider.java ef595c8
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PermissionFactory.java PRE-CREATION
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/ResourceAuthorizationProvider.java 60282e6
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/Roles.java 924c2cc
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/RolesFactory.java PRE-CREATION
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/ServerNameMustMatch.java 1d2a8c6
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/ServersAllIsInvalid.java 8ee1c43
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimplePolicyEngine.java 0d4c858
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimplePolicyParser.java PRE-CREATION
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/WildcardPermission.java 23c845d
> sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestAuthorizables.java f81b574
> sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestDatabaseRequiredInRole.java fc35043
> sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestPolicyParsingNegative.java 7285806
> sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestResourceAuthorizationProviderGeneralCases.java a8a946d
> sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestResourceAuthorizationProviderSpecialCases.java 14e2ff5
> sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestSimplePolicyEngineDFS.java 656a0fa
> sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestSimplePolicyEngineLocalFS.java 73cd673
> sentry-provider/sentry-provider-file/src/test/java/org/apache/sentry/provider/file/TestWildcardPermission.java 156aa64
> sentry-provider/sentry-provider-file/src/test/resources/test-authz-provider-other-group.ini cd3695c
> sentry-provider/sentry-provider-file/src/test/resources/test-authz-provider.ini 2d00699
> sentry-provider/sentry-provider-policy-db/pom.xml PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/AbstractDBRoleValidator.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/DBAuthorizables.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/DBRoles.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/DBWildcardPermission.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/DatabaseMustMatch.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/DatabaseRequiredInRole.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/HadoopGroupResourceAuthorizationProvider.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/LocalGroupResourceAuthorizationProvider.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/ServerNameMustMatch.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/ServersAllIsInvalid.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/main/java/org/apache/sentry/provider/db/SimpleDBPolicyEngine.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestDBAuthorizables.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestDBWildcardPermission.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestDatabaseRequiredInRole.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestPolicyParsingNegative.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestResourceAuthorizationProviderGeneralCases.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestResourceAuthorizationProviderSpecialCases.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestSimpleDBPolicyEngineDFS.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/test/java/org/apache/sentry/provider/db/TestSimpleDBPolicyEngineLocalFS.java PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/test/resources/log4j.properties PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/test/resources/test-authz-provider-other-group.ini PRE-CREATION
> sentry-provider/sentry-provider-policy-db/src/test/resources/test-authz-provider.ini PRE-CREATION
> sentry-tests/sentry-tests-hive/pom.xml 27b45c0
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPerDBConfiguration.java 17f4de1
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 288a7b3
>
> Diff: https://reviews.apache.org/r/14344/diff/
>
>
> Testing
> -------
>
> Ran "mvn test -Pdownload-hadoop" successfully.
>
>
> Thanks,
>
> Gregory Chanan
>
>