You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Arun Suresh <ar...@gmail.com> on 2014/05/29 00:54:49 UTC
Review Request 21995: SENTRY-157 : Support filter pushdown in DB Store
client to reduce data transfer from DB Store service
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21995/
-----------------------------------------------------------
Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
Repository: sentry
Description
-------
Patch for SENTRY-157(https://issues.apache.org/jira/browse/SENTRY-157)
To support push-down of Authorizable Hierarchy
Diffs
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java faa71c7
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java c378a38
sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java a95ef7b
sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java 4625d6f
sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java e88ae32
sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java 08f84a3
sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java 8adcb6f
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 1b0aba6
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java a175245
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java e1e7f4a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ece740b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 13af593
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java 82d701f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8f0ecfd
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 5f8a65f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8e58202
sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift a8b511d
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 67b05e6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java f51f518
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java 6e8f02f
Diff: https://reviews.apache.org/r/21995/diff/
Testing
-------
Thanks,
Arun Suresh
Re: Review Request 21995: SENTRY-157 : Support filter pushdown in DB Store
client to reduce data transfer from DB Store service
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
> On May 30, 2014, 6:14 p.m., Arun Suresh wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java, line 202
> > <https://reviews.apache.org/r/21995/diff/2/?file=599550#file599550line202>
> >
> > As per current logic, we will be doing 1 RPC per "authorizable Hierachy".. not an authorizable object..
> >
> > I agree, Join of multiple tables.. i.e sending multiple hierarchies would require us to create a new Hierarchy Datastructure (a list of list perhaps)... as a simple list will not suffice..
> >
> > But Id prefer working on it in follow-up JIRAs if you don't mind..
> >
> > I have included some integration tests in TestSEntryIntegration.. I can add somemore if required.. do let me know if I missed some usecases..
Ah, ok. I was under the impression that each authorizable corresponds to a heirarchy like "server=server1->db=default". So, I think we are good here. Thanks for the clarification!
- Sravya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21995/#review44395
-----------------------------------------------------------
On May 30, 2014, 1:27 a.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21995/
> -----------------------------------------------------------
>
> (Updated May 30, 2014, 1:27 a.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Patch for SENTRY-157(https://issues.apache.org/jira/browse/SENTRY-157)
>
> To support push-down of Authorizable Hierarchy
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java f1e6247
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java c378a38
> sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java a95ef7b
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java 4625d6f
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java e88ae32
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java 08f84a3
> sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java 8adcb6f
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 1b0aba6
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java a175245
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java e1e7f4a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ece740b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 54c1d6d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java 952ee78
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7e2323c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 2aac409
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java f92c78a
> sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift b4281c7
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 67b05e6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 56dcaf9
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java 6e8f02f
>
> Diff: https://reviews.apache.org/r/21995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 21995: SENTRY-157 : Support filter pushdown in DB Store
client to reduce data transfer from DB Store service
Posted by Arun Suresh <ar...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21995/#review44395
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java
<https://reviews.apache.org/r/21995/#comment78752>
As per current logic, we will be doing 1 RPC per "authorizable Hierachy".. not an authorizable object..
I agree, Join of multiple tables.. i.e sending multiple hierarchies would require us to create a new Hierarchy Datastructure (a list of list perhaps)... as a simple list will not suffice..
But Id prefer working on it in follow-up JIRAs if you don't mind..
I have included some integration tests in TestSEntryIntegration.. I can add somemore if required.. do let me know if I missed some usecases..
- Arun Suresh
On May 30, 2014, 1:27 a.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21995/
> -----------------------------------------------------------
>
> (Updated May 30, 2014, 1:27 a.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Patch for SENTRY-157(https://issues.apache.org/jira/browse/SENTRY-157)
>
> To support push-down of Authorizable Hierarchy
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java f1e6247
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java c378a38
> sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java a95ef7b
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java 4625d6f
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java e88ae32
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java 08f84a3
> sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java 8adcb6f
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 1b0aba6
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java a175245
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java e1e7f4a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ece740b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 54c1d6d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java 952ee78
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7e2323c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 2aac409
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java f92c78a
> sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift b4281c7
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 67b05e6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 56dcaf9
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java 6e8f02f
>
> Diff: https://reviews.apache.org/r/21995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 21995: SENTRY-157 : Support filter pushdown in DB Store
client to reduce data transfer from DB Store service
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21995/#review44372
-----------------------------------------------------------
Thanks for the update Arun! I just realized that the way you are building the authorizable object for RPC needs some work(details in the comment). Otherwise it looks good.
As I thought a little more about it, I feel we still have some places(some unrelated to this patch as such) where we can clean up/improve, but lets do them as multiple follow on jiras.
1. Getting rid of maintaining active role set struct and list_sentry_privileges_for_provider in thrift
I think we should handle active roles on hive side outside of sentry service, as we do not really store these mappings in the db. And does not make sense to store these in db as these are per session variables. If we do this, we can clean up the thrift interface a bit and just have:
TListSentryPrivilegesResponse list_sentry_privileges(1:TListSentryPrivilegesRequest request)
struct TListSentryPrivilegesRequest {
1: required i32 protocol_version = sentry_common_service.TSENTRY_SERVICE_V1,
2: required string requestorUserName, # user on whose behalf the request is issued
3: required set<string> roleNames # get privileges assigned for this role
4: optional TSentryAuthorizable authorizableHierarchy
}
And we will do the set intersection of rolesforGroup and active roles in the hive binding.
2. Accept list of authorizables: Set<TSentryAuthorizable> authorizables
Hive commands can involve multiple authorizable objects, for example for joins, we need permissions on two tables. Today we will have to do multiple RPCs.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/21995/#comment78729>
wonder why this was final in the first place? And how did anything work with it :-)
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java
<https://reviews.apache.org/r/21995/#comment78744>
We will have to do one RPC for each authorizable. Or change the RPC to accept list of authorizables. Imagine we have a join of two tables, in which case our authorizables {(db1,tb1), (db2,tb2), in which case current logic fails. Also would be good to add some end to end tests, we can do that as a follow if you prefer after SENTRY-153 is committed.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java
<https://reviews.apache.org/r/21995/#comment78745>
Same as above
- Sravya Tirukkovalur
On May 30, 2014, 1:27 a.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21995/
> -----------------------------------------------------------
>
> (Updated May 30, 2014, 1:27 a.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Patch for SENTRY-157(https://issues.apache.org/jira/browse/SENTRY-157)
>
> To support push-down of Authorizable Hierarchy
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java f1e6247
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java c378a38
> sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java a95ef7b
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java 4625d6f
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java e88ae32
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java 08f84a3
> sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java 8adcb6f
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 1b0aba6
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java a175245
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java e1e7f4a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ece740b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 54c1d6d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java 952ee78
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7e2323c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 2aac409
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java f92c78a
> sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift b4281c7
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 67b05e6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 56dcaf9
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java 6e8f02f
>
> Diff: https://reviews.apache.org/r/21995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 21995: SENTRY-157 : Support filter pushdown in DB Store
client to reduce data transfer from DB Store service
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21995/#review44447
-----------------------------------------------------------
Ship it!
Ship It!
- Sravya Tirukkovalur
On May 30, 2014, 10:49 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21995/
> -----------------------------------------------------------
>
> (Updated May 30, 2014, 10:49 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Patch for SENTRY-157(https://issues.apache.org/jira/browse/SENTRY-157)
>
> To support push-down of Authorizable Hierarchy
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java f1e6247
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java c378a38
> sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java a95ef7b
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java 4625d6f
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java e88ae32
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java 08f84a3
> sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java 8adcb6f
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 1b0aba6
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java a175245
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java e1e7f4a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ece740b
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryPrivilegesForProviderRequest.java 65fd5b5
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryPrivilegesRequest.java 89afb70
> sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryAuthorizable.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 54c1d6d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java 952ee78
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7e2323c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 2aac409
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java f92c78a
> sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift b4281c7
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 67b05e6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 56dcaf9
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java 6e8f02f
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java 1d89e1a
>
> Diff: https://reviews.apache.org/r/21995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 21995: SENTRY-157 : Support filter pushdown in DB Store
client to reduce data transfer from DB Store service
Posted by Arun Suresh <ar...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21995/
-----------------------------------------------------------
(Updated May 30, 2014, 10:49 p.m.)
Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
Updated Patch file with test cases fixes...
Repository: sentry
Description
-------
Patch for SENTRY-157(https://issues.apache.org/jira/browse/SENTRY-157)
To support push-down of Authorizable Hierarchy
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java f1e6247
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java c378a38
sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java a95ef7b
sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java 4625d6f
sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java e88ae32
sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java 08f84a3
sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java 8adcb6f
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 1b0aba6
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java a175245
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java e1e7f4a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ece740b
sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryPrivilegesForProviderRequest.java 65fd5b5
sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryPrivilegesRequest.java 89afb70
sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryAuthorizable.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 54c1d6d
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java 952ee78
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7e2323c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 2aac409
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java f92c78a
sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift b4281c7
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 67b05e6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 56dcaf9
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java 6e8f02f
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbEndToEnd.java 1d89e1a
Diff: https://reviews.apache.org/r/21995/diff/
Testing
-------
Thanks,
Arun Suresh
Re: Review Request 21995: SENTRY-157 : Support filter pushdown in DB Store
client to reduce data transfer from DB Store service
Posted by Arun Suresh <ar...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21995/
-----------------------------------------------------------
(Updated May 30, 2014, 1:27 a.m.)
Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
Updated diff with comments and rebased with HEAD
Repository: sentry
Description
-------
Patch for SENTRY-157(https://issues.apache.org/jira/browse/SENTRY-157)
To support push-down of Authorizable Hierarchy
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java f1e6247
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java c378a38
sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java a95ef7b
sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java 4625d6f
sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java e88ae32
sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java 08f84a3
sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java 8adcb6f
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 1b0aba6
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java a175245
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java e1e7f4a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ece740b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 54c1d6d
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java 952ee78
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 7e2323c
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 2aac409
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java f92c78a
sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift b4281c7
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 67b05e6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java 56dcaf9
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java 6e8f02f
Diff: https://reviews.apache.org/r/21995/diff/
Testing
-------
Thanks,
Arun Suresh
Re: Review Request 21995: SENTRY-157 : Support filter pushdown in DB Store
client to reduce data transfer from DB Store service
Posted by Arun Suresh <ar...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21995/#review44244
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/21995/#comment78582>
True... will make the change...
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/21995/#comment78581>
Hmmm... not really sure..
Firstly, I wanted a query that returns Privileges, not Roles and did not want the overhead of loading the entire Role rows..
Also, my understanding is that a JDO query on MSentryRole.class and then doing a getPrivileges on the returned object will result in JDO firing a select query that queries ALL privileges.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/21995/#comment78583>
will do.. nice spotting
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/21995/#comment78608>
Yeah.. this method can be cleaned up based on your previous comment.. thanx
- Arun Suresh
On May 28, 2014, 10:54 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21995/
> -----------------------------------------------------------
>
> (Updated May 28, 2014, 10:54 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Patch for SENTRY-157(https://issues.apache.org/jira/browse/SENTRY-157)
>
> To support push-down of Authorizable Hierarchy
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java faa71c7
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java c378a38
> sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java a95ef7b
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java 4625d6f
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java e88ae32
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java 08f84a3
> sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java 8adcb6f
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 1b0aba6
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java a175245
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java e1e7f4a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ece740b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 13af593
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java 82d701f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8f0ecfd
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 5f8a65f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8e58202
> sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift a8b511d
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 67b05e6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java f51f518
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java 6e8f02f
>
> Diff: https://reviews.apache.org/r/21995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 21995: SENTRY-157 : Support filter pushdown in DB Store
client to reduce data transfer from DB Store service
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21995/#review44236
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/21995/#comment78572>
Seems like there is code duplication. Change getRoleToPrivilegeMap(Groups) to getMSentryPrivileges(RoleNames, Filter=none) instead and reuse the data nucleus querying part?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/21995/#comment78577>
I think it will be faster to do a look up on MSentryRole.class, as roleName would be indexed? Similar to getMSentryRoleByName()?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/21995/#comment78569>
Nit: nust -> must
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/21995/#comment78571>
Not related to this patch, but it looks like we allow "set role roleName", even if the user is not granted that role. But do the intersection while querying to avoid using non granted roles. We should probably throw an error when trying to set a non granted role?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/21995/#comment78570>
I would also do a set intersection of activeRoleNames and roleNamesForGroups here. And actually pass this result to getRoleToPrivilege mapping instead of "groups".
- Sravya Tirukkovalur
On May 28, 2014, 10:54 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21995/
> -----------------------------------------------------------
>
> (Updated May 28, 2014, 10:54 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Patch for SENTRY-157(https://issues.apache.org/jira/browse/SENTRY-157)
>
> To support push-down of Authorizable Hierarchy
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java faa71c7
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java c378a38
> sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java a95ef7b
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java 4625d6f
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java e88ae32
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java 08f84a3
> sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java 8adcb6f
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java 1b0aba6
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java a175245
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java e1e7f4a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ece740b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 13af593
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java 82d701f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 8f0ecfd
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 5f8a65f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 8e58202
> sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift a8b511d
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 67b05e6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java f51f518
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java 6e8f02f
>
> Diff: https://reviews.apache.org/r/21995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Arun Suresh
>
>