You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Na Li via Review Board <no...@reviews.apache.org> on 2019/12/15 06:33:15 UTC
Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
Diff: https://reviews.apache.org/r/71915/diff/1/
Testing
-------
Performance test in
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
> > Line 852 (original), 852 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189390#file2189390line852>
> >
> > Can we make this change configurable? Right now, it seems that there is no way a user can revert to old behavior in case of any issues with the TreePrivilegeCache.
done
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Line 503 (original), 503 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189391#file2189391line503>
> >
> > Make this configurable?
done
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
> > Lines 33 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189404#file2189404line33>
> >
> > It looks like adding any new methods to this interface will break the clients who implement this? Is this not a public API? If yes, can we make changes to avoid backwards incompatibility?
move the new functions to a child interface of PrivilegeCache
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219056
-----------------------------------------------------------
On Dec. 18, 2019, 4:52 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2019, 4:52 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
>
>
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
>
> We should add a new function that returns privilege object directly to avoid the overhead of conversion.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
>
>
> Diff: https://reviews.apache.org/r/71915/diff/6/
>
>
> Testing
> -------
>
> Performance test in
>
> 1)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[1]
>
> TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
> 2)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[10]
>
> TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
> > Lines 269 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189392#file2189392line269>
> >
> > not sure I understand this. Can you clarify why is this related to this patch?
In this function SentryMetaStoreFilterHook.filterTab(), you can see authzBinding.close() is called for each filtering operation. If its cache is cleared at close() and the same HiveMetaStoreClient is used for more than one filtering operation, the next filtering operation will fail.
In TreePrivilegeCache.close(), the cachedPrivileges and cachedPrivilegeMap are cleared. So there is no cached privileges, and the test TestMetastoreEndToEnd.testListTables failed when the same HiveMetaStoreClient is used to filter more than once.
On the other hand, SimplePrivilegeCache.close(), the cachedPrivileges is cleared, but cachedAuthzPrivileges is not cleared. That is why the test TestMetastoreEndToEnd.testListTables did not fail for it.
I believe the correct behavior for cache is to clear its cache at close(). Therefore, we need to make sure SentryMetaStoreFilterHook.filterTab() will clear its hiveAuthzBinding, and get new privileges for its cache in next operation.
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
> > Lines 26-27 (original), 26-27 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189397#file2189397line26>
> >
> > Do you think we should intern these? Seems like there would be a lot of duplicates otherwise.
done
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
> > Lines 42 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189407#file2189407line42>
> >
> > Do we need to cache strings as well since we are generating CachedPrivilegeMap?
it is there to suport old API such as "listPrivileges(Set<String> groups, ActiveRoleSet roleSet)"
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 48 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line48>
> >
> > change inPrivilege to final?
done
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 113 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line113>
> >
> > targetSet seems to be a local variable on the stack. Why do we need to create a copyOf it?
You are right. There is no need. removed the copy.
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 116 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line116>
> >
> > This method has exact sane signature of with listPrivilegeObjects. What is the difference between the two?
added comment to show the different functionalities.
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 126 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line126>
> >
> > Do we have to distinguish the cases for different wildcards?
Yes. When the authorizable part is wildcard, it should get privileges of all children. For example, in "show tables" command, the cache should return all column level privileges. The column = "*"
requiredInputPrivileges [SELECT, INSERT, ALTER, CREATE, DROP, INDEX, LOCK]
inputHierarchyList = [[Server [name=server1], Database [name=db_1], Table [name=tab1], Column [name=*]]]
If the authorizable part is not wildcard, it should only get privileges of that specific child.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219056
-----------------------------------------------------------
On Dec. 18, 2019, 4:52 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2019, 4:52 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
>
>
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
>
> We should add a new function that returns privilege object directly to avoid the overhead of conversion.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
>
>
> Diff: https://reviews.apache.org/r/71915/diff/6/
>
>
> Testing
> -------
>
> Performance test in
>
> 1)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[1]
>
> TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
> 2)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[10]
>
> TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 163 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line163>
> >
> > please add comments int this method with examples for clarity. Also not very clear on why the index is offset by 1.
I replaced the check with the function hasChild(). The index is 0 based. So if the array size is 2, and the index value is 1, then the index already points to the end of the array. That is why there is offset by 1.
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 183 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line183>
> >
> > I think the usage of the word "key" in method/variable names for both authorizable and Privilege is very confusing. Can you please rename the variables and method names better?
I changed the function names to getChildResourceValue() and isResourceValueWildcard(). Hopefully it is easier to read the code now.
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 204 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line204>
> >
> > may be rename this to getChildAuthorizableName?
getChildResourceValue()
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 222 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line222>
> >
> > The method name suggests its returning the key but the implememtation returns the value. Can you please add a java doc on these methods to improve readability?
change the function name to getChildResourceValue(), which is used as key for the hashmap.
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
> > Lines 229 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189408#file2189408line229>
> >
> > Its unclear why we are using partIndex+1. Can you add some comments to explain the same?
replace it with hasChild()
> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java
> > Line 58 (original), 66 (patched)
> > <https://reviews.apache.org/r/71915/diff/6/?file=2189410#file2189410line66>
> >
> > Why was this change necessary?
the type of a column authorizable is "column". It is a typo to put the type as "col". The error was found after I added new test cases below
assertEquals(5, cache.listPrivileges(null, null, null, new Server("server1"), new Database("db1"), new Table("t1"), new Column("*")).size());
assertEquals(6, cache.listPrivileges(null, null, null, new Server("server1"), new Database("db1"), new Table("*"), new Column("*")).size());
assertEquals(2, cache.listPrivileges(null, null, null, new Server("server1"), new Database("db2"), new Table("t1"), new Column("*")).size());
1) The authorizable string is "server=server1->db=db1->table=t1->column=*" for `new Server("server1"), new Database("db1"), new Table("t1"), new Column("*")`
2) With the typo,
2.1) the privilege string is "server=server1->db=db1->table=t1->col=c1->action=select" for the privilege
CommonPrivilege colSelect = create(new KeyValue("Server", "server1"),
new KeyValue("db", "db1"), new KeyValue("table", "t1"), new KeyValue("col", "c1"), new KeyValue("action", "SELECT"));
2.2) This privilege does not match the input authorizable string "col" vs "column"
3) After fixing the typo,
3.1) the privilege string is "server=server1->db=db1->table=t1->column=c1->action=select" for the privilege
CommonPrivilege colSelect = create(new KeyValue("Server", "server1"),
new KeyValue("db", "db1"), new KeyValue("table", "t1"), new KeyValue("column", "c1"), new KeyValue("action", "SELECT"));
3.2) This privilege does match the input authorizable string "column" vs "column"
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219056
-----------------------------------------------------------
On Dec. 18, 2019, 4:52 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2019, 4:52 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
>
>
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
>
> We should add a new function that returns privilege object directly to avoid the overhead of conversion.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
>
>
> Diff: https://reviews.apache.org/r/71915/diff/6/
>
>
> Testing
> -------
>
> Performance test in
>
> 1)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[1]
>
> TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
> 2)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[10]
>
> TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Vihang Karajgaonkar via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219056
-----------------------------------------------------------
I took a first pass at this and left some comments. I think someone who is more familiar with Sentry should take a look at this as well.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
Line 852 (original), 852 (patched)
<https://reviews.apache.org/r/71915/#comment307095>
Can we make this change configurable? Right now, it seems that there is no way a user can revert to old behavior in case of any issues with the TreePrivilegeCache.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Line 503 (original), 503 (patched)
<https://reviews.apache.org/r/71915/#comment307096>
Make this configurable?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
Lines 269 (patched)
<https://reviews.apache.org/r/71915/#comment307097>
not sure I understand this. Can you clarify why is this related to this patch?
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java
Lines 26-27 (original), 26-27 (patched)
<https://reviews.apache.org/r/71915/#comment307100>
Do you think we should intern these? Seems like there would be a lot of duplicates otherwise.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
Lines 33 (patched)
<https://reviews.apache.org/r/71915/#comment307084>
It looks like adding any new methods to this interface will break the clients who implement this? Is this not a public API? If yes, can we make changes to avoid backwards incompatibility?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 42 (patched)
<https://reviews.apache.org/r/71915/#comment307099>
Do we need to cache strings as well since we are generating CachedPrivilegeMap?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
Lines 48 (patched)
<https://reviews.apache.org/r/71915/#comment307088>
change inPrivilege to final?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
Lines 113 (patched)
<https://reviews.apache.org/r/71915/#comment307091>
targetSet seems to be a local variable on the stack. Why do we need to create a copyOf it?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
Lines 116 (patched)
<https://reviews.apache.org/r/71915/#comment307090>
This method has exact sane signature of with listPrivilegeObjects. What is the difference between the two?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
Lines 126 (patched)
<https://reviews.apache.org/r/71915/#comment307089>
Do we have to distinguish the cases for different wildcards?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
Lines 163 (patched)
<https://reviews.apache.org/r/71915/#comment307087>
please add comments int this method with examples for clarity. Also not very clear on why the index is offset by 1.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
Lines 183 (patched)
<https://reviews.apache.org/r/71915/#comment307093>
I think the usage of the word "key" in method/variable names for both authorizable and Privilege is very confusing. Can you please rename the variables and method names better?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
Lines 204 (patched)
<https://reviews.apache.org/r/71915/#comment307092>
may be rename this to getChildAuthorizableName?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
Lines 222 (patched)
<https://reviews.apache.org/r/71915/#comment307086>
The method name suggests its returning the key but the implememtation returns the value. Can you please add a java doc on these methods to improve readability?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
Lines 229 (patched)
<https://reviews.apache.org/r/71915/#comment307085>
Its unclear why we are using partIndex+1. Can you add some comments to explain the same?
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java
Line 58 (original), 66 (patched)
<https://reviews.apache.org/r/71915/#comment307094>
Why was this change necessary?
- Vihang Karajgaonkar
On Dec. 18, 2019, 4:52 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2019, 4:52 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
>
>
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
>
> We should add a new function that returns privilege object directly to avoid the overhead of conversion.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
>
>
> Diff: https://reviews.apache.org/r/71915/diff/6/
>
>
> Testing
> -------
>
> Performance test in
>
> 1)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[1]
>
> TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
> 2)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[10]
>
> TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On Dec. 20, 2019, 11:07 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
> > Lines 97 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191022#file2191022line97>
> >
> > do we need copyOf?
How about using Collections.unmodifiableSet(filteredPrivilegeCache.listPrivilegeObjects(groups, users, roleSet, authorizableHierarchy)));
This avoids creating new copy of the collection for each call. It still makes sure that the caller will not be able to modify the list.
Using Collections.unmodifiableList creates a wrapper around your List. if the underlying list changes, so does your unmodifiableList's view.
Creating copy is not efficient. It's a more expensive computation and consumes more memor.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219090
-----------------------------------------------------------
On Dec. 20, 2019, 9:27 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2019, 9:27 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
>
>
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
>
> We should add a new function that returns privilege object directly to avoid the overhead of conversion.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 90fcfc3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
>
>
> Diff: https://reviews.apache.org/r/71915/diff/9/
>
>
> Testing
> -------
>
> Performance test in
>
> 1)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[1]
>
> TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
> 2)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[10]
>
> TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Dec. 20, 2019, 11:07 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java
> > Lines 35 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191020#file2191020line35>
> >
> > I think instead of setPrivilegeFactory, we should just have PrivilegeFactory getPrivilegeFactory(). That would mean that we can make privilegeFactory final field in the cache. I am not sure if we would want to have privilegeFactory to be configurable once the instance of the cache is created.
remove this function, and let privilege factory be an input to create a filtered cache
> On Dec. 20, 2019, 11:07 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
> > Line 46 (original)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191021#file2191021line46>
> >
> > do we need to remove this? This nullifies the objective of creating a separate interface FilteredPrivilegeCache
yes. Removing this function makes PrivilegeCache the same as its original content before SENTRY-1291. And the changes from SENTRY-1291 and SENTRY-2539 are only in FilteredPrivilegeCache, which adds another functionlistPrivilegeObjects
> On Dec. 20, 2019, 11:07 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
> > Lines 97 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191022#file2191022line97>
> >
> > do we need copyOf?
>
> kalyan kumar kalvagadda wrote:
> How about using Collections.unmodifiableSet(filteredPrivilegeCache.listPrivilegeObjects(groups, users, roleSet, authorizableHierarchy)));
> This avoids creating new copy of the collection for each call. It still makes sure that the caller will not be able to modify the list.
> Using Collections.unmodifiableList creates a wrapper around your List. if the underlying list changes, so does your unmodifiableList's view.
>
> Creating copy is not efficient. It's a more expensive computation and consumes more memor.
Collections.unmodifiableSet() returns type of Set<Privilege>, however, the returned type should be ImmutableSet<Privilege>. See this link for more info.
https://stackoverflow.com/questions/5611324/whats-the-difference-between-collections-unmodifiableset-and-immutableset-of
an important difference between ImmutableSet and the Set created by Collections.unmodifiableSet is that ImmutableSet is a type
> On Dec. 20, 2019, 11:07 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
> > Lines 101 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191022#file2191022line101>
> >
> > instead of returning empty may be throw a UnsupportedOperationException. Also you can add a Preconditions.checkState(cacheHandle instanceOf FilteredPrivilegeCache) in such a case.
The caller ResourceAuthorizationProvider does not know what type of cache is used. If the cache does not support getPrivilegeObjects, we don't want to throw exception for each authorization check. it is too expensive. Instead, the caller should call getPrivileges() to get corresponding privileges
> On Dec. 20, 2019, 11:07 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java
> > Lines 41 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191023#file2191023line41>
> >
> > If you don't modify the existing PrivilegeCache interface, we don't need this implementation. We should keep SimplePrivilegeCache as is so that fallback to previous behavior is possible.
We have three implementations of the privilege caches.
1.1) org.apache.sentry.provider.cache.SimplePrivilegeCache (the original cache implementation before SENTRY-1291. This is what customers use right now, and they are having performance issue. If customers have issue using TreePrivilegeCache, they can fall back to this original SimplePrivilegeCache)
1.2) org.apache.sentry.provider.cache.SimpleFilteredPrivilegeCache (the cache implemented in SENTRY-1291 with class name of SimplePrivilegeCache. Its implementation is now moved to SimpleFilteredPrivilegeCache. It is not used by big customers.)
1.3) org.apache.sentry.provider.cache.TreePrivilegeCache (the cache implemented in this Jira SENTRY-2539. It should be used to reduce performance issue.)
> On Dec. 20, 2019, 11:07 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java
> > Lines 58 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191023#file2191023line58>
> >
> > can we use privilegeFactory here to create privilege instead of directly CommonPrivilege instance?
fixed.
> On Dec. 20, 2019, 11:07 p.m., Vihang Karajgaonkar wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
> > Lines 49 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191025#file2191025line49>
> >
> > The code assumes that when the cache is being created all the privileges which are being cached are available upfront. But the general pattern of a privilege cache is such that it should support addPrivilege() and removePrivilege() methods.
The scenarios that this cache is used are when the cache is being created all the privileges which are being cached are available upfront. This is not assumption. This is the reality.
In hive, sentry client gets all privileges of that user for each command, and creates cache to store it. So all authorization checks for this command are using this cache. https://github.com/apache/sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java#L852
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219090
-----------------------------------------------------------
On Dec. 20, 2019, 9:27 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2019, 9:27 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
>
>
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
>
> We should add a new function that returns privilege object directly to avoid the overhead of conversion.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 90fcfc3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
>
>
> Diff: https://reviews.apache.org/r/71915/diff/9/
>
>
> Testing
> -------
>
> Performance test in
>
> 1)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[1]
>
> TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
> 2)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[10]
>
> TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Vihang Karajgaonkar via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219090
-----------------------------------------------------------
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java
Lines 35 (patched)
<https://reviews.apache.org/r/71915/#comment307158>
I think instead of setPrivilegeFactory, we should just have PrivilegeFactory getPrivilegeFactory(). That would mean that we can make privilegeFactory final field in the cache. I am not sure if we would want to have privilegeFactory to be configurable once the instance of the cache is created.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
Line 46 (original)
<https://reviews.apache.org/r/71915/#comment307155>
do we need to remove this? This nullifies the objective of creating a separate interface FilteredPrivilegeCache
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
Lines 97 (patched)
<https://reviews.apache.org/r/71915/#comment307156>
do we need copyOf?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
Lines 101 (patched)
<https://reviews.apache.org/r/71915/#comment307159>
instead of returning empty may be throw a UnsupportedOperationException. Also you can add a Preconditions.checkState(cacheHandle instanceOf FilteredPrivilegeCache) in such a case.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java
Lines 41 (patched)
<https://reviews.apache.org/r/71915/#comment307160>
If you don't modify the existing PrivilegeCache interface, we don't need this implementation. We should keep SimplePrivilegeCache as is so that fallback to previous behavior is possible.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java
Lines 58 (patched)
<https://reviews.apache.org/r/71915/#comment307157>
can we use privilegeFactory here to create privilege instead of directly CommonPrivilege instance?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 49 (patched)
<https://reviews.apache.org/r/71915/#comment307164>
The code assumes that when the cache is being created all the privileges which are being cached are available upfront. But the general pattern of a privilege cache is such that it should support addPrivilege() and removePrivilege() methods.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 57 (patched)
<https://reviews.apache.org/r/71915/#comment307161>
Is the ability to set the privilegeFactory is necessary? Instead can we pass the PrivilegeFactory as an argument to the constructor and make it final. That would mean that we only need a getPrivilegeFactory method in the in FilteredPrivilegeCache interface method instead of setPrivilegeFactory.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 60 (patched)
<https://reviews.apache.org/r/71915/#comment307162>
The reason I suggest not to have a setPrivilegeFactory is because setting the privilegeFactory is actually recreating the cache which is an unusual pattern to use.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 66-79 (patched)
<https://reviews.apache.org/r/71915/#comment307167>
Why don't we use the input arguments here?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 67 (patched)
<https://reviews.apache.org/r/71915/#comment307163>
Instead of doing multiple null checks for this variable, may be make sure that it is never null initializing this to a HashSet<>() and making it final.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 93 (patched)
<https://reviews.apache.org/r/71915/#comment307170>
why we don't use the groups, users and roleSet arguments. Looks like this API is returning all the privileges for all the users for the given authorizationheirarchy. Same with the above method.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 95 (patched)
<https://reviews.apache.org/r/71915/#comment307168>
In the other method calls we are checking if the cachedPrivileges is null we return a empty result. May be we should do the same thing to be consistent. Since this class is not expected to be thread-safe anyways, why do we need to handle this case? I mean, will there be any case where cachedPrivileges is not null while cachedPrivilegeMap is null?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 120 (patched)
<https://reviews.apache.org/r/71915/#comment307171>
Do we expect the clients to call listPrivileges after close? If not, we should enforce that in code by throwing a IllegalStateException once this is closed.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 174 (patched)
<https://reviews.apache.org/r/71915/#comment307169>
can we be consistent. One method takes index first while the other takes it second argument.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
Lines 44 (patched)
<https://reviews.apache.org/r/71915/#comment307165>
Isn't this partIndex = 1 since partIndex=0 is server1?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java
Lines 78 (patched)
<https://reviews.apache.org/r/71915/#comment307166>
Should this be an exception instead of warn?
- Vihang Karajgaonkar
On Dec. 20, 2019, 9:27 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2019, 9:27 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
>
>
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
>
> We should add a new function that returns privilege object directly to avoid the overhead of conversion.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 90fcfc3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
>
>
> Diff: https://reviews.apache.org/r/71915/diff/9/
>
>
> Testing
> -------
>
> Performance test in
>
> 1)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[1]
>
> TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
> 2)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[10]
>
> TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
> > Lines 233-236 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191008#file2191008line233>
> >
> > Did you see any issue with out this change?
yes. without this change, if the same hook is called the second time to filter databases or tables, the privileges are not refectched, and may not be up-to-date.
> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
> > Line 46 (original)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191021#file2191021line46>
> >
> > I agree.
this function was added by SENTRY-1291 https://reviews.apache.org/r/47872/diff/6#9.
I want to keep this Interface the same as before SENTRY-1291 was committed, and all changes in SENTRY-1291 and SENTRY-2539 in another interface to maintain backward compatibility.
In this way, if the changes in SENTRY-1291 and SENTRY-2539 cause problem, we can easily rollback to the behavior before SENTRY-1291 and SENTRY-2539.
> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
> > Lines 75-78 (original), 76-84 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191022#file2191022line76>
> >
> > Why should be do this up-casting?
> >
> > If the below API is not removed from Privilge cache we would need this special check.
> >
> > Set<String> listPrivileges(Set<String> groups, Set<String> users, ActiveRoleSet roleSet,
> > Authorizable... authorizationhierarchy);
> >
> >
> > Is there any reason to remove this API. This might even break Impala which implements this interface.
"Is there any reason to remove this API. This might even break Impala which implements this interface."
this function was added by SENTRY-1291 https://reviews.apache.org/r/47872/diff/6#9, and Impala rejected this change, and does NOT suppor this function. To avoid breaking Impala, we should not keep this function in Interface PrivilegeCache.java.
I want to keep the Interface PrivilegeCache.java the same as before SENTRY-1291 was committed, and all changes in SENTRY-1291 and SENTRY-2539 in another interface to maintain backward compatibility.
In this way, if the changes in SENTRY-1291 and SENTRY-2539 cause problem, we can easily rollback to the behavior before SENTRY-1291 and SENTRY-2539.
> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
> > Lines 170 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191025#file2191025line170>
> >
> > Second argument here is basically is partIndex.
> > You sending a -1 and using it by incrementing by one.
> > Instead, I think you should just pass 0.
If I pass 0 to the function getChildResourceValue(), the index will be 0 + 1 = 1, and the result will be wrong.
To make it work, I implemented the function getTopLevelResourceValue() that is even simpler, and does not need caller to pass in any index. The equivalent index is 0, which is (- 1 + 1) = 0.
> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
> > Lines 174 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191025#file2191025line174>
> >
> > First argument here is basically is partIndex.
> > You sending a -1 and using it by incrementing by one.
> > Instead, I think you should just pass 0.
> >
> >
> > +1 on vehang's comment.
I have added new function getResourceValue() to make code more readable.
> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
> > Lines 217-225 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191033#file2191033line217>
> >
> > Why are you converting the privilge object to string and performening the check?
> >
> > Something like
> >
> > private boolean hasOnlyServerPrivilege(Privilege privObj) {
> > if(privObj.getParts().size() == 1 && privObj.getParts().get(0).getKey().equalsIgnoreCase("server")) {
> > return privObj.getParts().get(0).getValue().endsWith("+");
> > }
> > return false;
> > }
Thanks for the suggestion. I did that before adding function getParts() in Privilege. After adding this function, I don't need to convert the privilege to string and can use your suggestion.
> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java
> > Lines 111-151 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191035#file2191035line111>
> >
> > Can you avoid code duplication?
removed duplication
> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
> > Lines 137-150 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191036#file2191036line137>
> >
> > This method is incomplete. If you intent that this API not be used adding a comment here may not help.
> >
> > @Override
> > public ImmutableSet<Privilege> getPrivilegeObjects(Set<String> groups, Set<String> users,
> > ActiveRoleSet roleSet, Authorizable... authorizableHierarchy) {
> > throw Exception(); // Appropriate exception
> > }
> >
> > This way if someone calls this method they would know that it is not supported.
I have added comment in the interface ProviderBackend.
Its caller is ResourceAuthorizationProvider, which does not know what cache to be used. Throwing exception for each authorization check is too expensive. ResourceAuthorizationProvider's behavior is to call getPrivilegeObjects(). If return set is empty, it will call getPrivileges()
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219091
-----------------------------------------------------------
On Dec. 20, 2019, 9:27 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2019, 9:27 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
>
>
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
>
> We should add a new function that returns privilege object directly to avoid the overhead of conversion.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 90fcfc3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
>
>
> Diff: https://reviews.apache.org/r/71915/diff/9/
>
>
> Testing
> -------
>
> Performance test in
>
> 1)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[1]
>
> TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
> 2)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[10]
>
> TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
> > Lines 170 (patched)
> > <https://reviews.apache.org/r/71915/diff/9/?file=2191025#file2191025line170>
> >
> > Second argument here is basically is partIndex.
> > You sending a -1 and using it by incrementing by one.
> > Instead, I think you should just pass 0.
>
> Na Li wrote:
> If I pass 0 to the function getChildResourceValue(), the index will be 0 + 1 = 1, and the result will be wrong.
>
> To make it work, I implemented the function getTopLevelResourceValue() that is even simpler, and does not need caller to pass in any index. The equivalent index is 0, which is (- 1 + 1) = 0.
added function getResourceValue() that can accept index value of 0.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219091
-----------------------------------------------------------
On Dec. 20, 2019, 9:27 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2019, 9:27 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
>
>
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
>
> We should add a new function that returns privilege object directly to avoid the overhead of conversion.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 90fcfc3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
>
>
> Diff: https://reviews.apache.org/r/71915/diff/9/
>
>
> Testing
> -------
>
> Performance test in
>
> 1)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[1]
>
> TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
> 2)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[10]
>
> TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219091
-----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
Lines 233-236 (patched)
<https://reviews.apache.org/r/71915/#comment307180>
Did you see any issue with out this change?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
Line 46 (original)
<https://reviews.apache.org/r/71915/#comment307181>
I agree.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
Lines 75-78 (original), 76-84 (patched)
<https://reviews.apache.org/r/71915/#comment307175>
Why should be do this up-casting?
If the below API is not removed from Privilge cache we would need this special check.
Set<String> listPrivileges(Set<String> groups, Set<String> users, ActiveRoleSet roleSet,
Authorizable... authorizationhierarchy);
Is there any reason to remove this API. This might even break Impala which implements this interface.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 170 (patched)
<https://reviews.apache.org/r/71915/#comment307172>
Second argument here is basically is partIndex.
You sending a -1 and using it by incrementing by one.
Instead, I think you should just pass 0.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 174 (patched)
<https://reviews.apache.org/r/71915/#comment307173>
First argument here is basically is partIndex.
You sending a -1 and using it by incrementing by one.
Instead, I think you should just pass 0.
+1 on vehang's comment.
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
Lines 217-225 (patched)
<https://reviews.apache.org/r/71915/#comment307177>
Why are you converting the privilge object to string and performening the check?
Something like
private boolean hasOnlyServerPrivilege(Privilege privObj) {
if(privObj.getParts().size() == 1 && privObj.getParts().get(0).getKey().equalsIgnoreCase("server")) {
return privObj.getParts().get(0).getValue().endsWith("+");
}
return false;
}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java
Lines 111-151 (patched)
<https://reviews.apache.org/r/71915/#comment307178>
Can you avoid code duplication?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
Lines 137-150 (patched)
<https://reviews.apache.org/r/71915/#comment307179>
This method is incomplete. If you intent that this API not be used adding a comment here may not help.
@Override
public ImmutableSet<Privilege> getPrivilegeObjects(Set<String> groups, Set<String> users,
ActiveRoleSet roleSet, Authorizable... authorizableHierarchy) {
throw Exception(); // Appropriate exception
}
This way if someone calls this method they would know that it is not supported.
- kalyan kumar kalvagadda
On Dec. 20, 2019, 9:27 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2019, 9:27 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
>
>
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
>
> We should add a new function that returns privilege object directly to avoid the overhead of conversion.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 90fcfc3
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
>
>
> Diff: https://reviews.apache.org/r/71915/diff/9/
>
>
> Testing
> -------
>
> Performance test in
>
> 1)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[1]
>
> TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
> 2)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[10]
>
> TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219106
-----------------------------------------------------------
Ship it!
Ship It!
- kalyan kumar kalvagadda
On Dec. 22, 2019, 9:45 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71915/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2019, 9:45 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
>
>
> Bugs: sentry-2359
> https://issues.apache.org/jira/browse/sentry-2359
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
>
> We should add a new function that returns privilege object directly to avoid the overhead of conversion.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 8f45c60
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
> sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
> sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
> sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
> sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java 9045773
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
>
>
> Diff: https://reviews.apache.org/r/71915/diff/11/
>
>
> Testing
> -------
>
> Performance test in
>
> 1)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[1]
>
> TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
> 2)
> Privilege: db[1000], table[10]
> Authorizable: db[12000], table[10]
>
> TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
> TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
>
> SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
> SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
(Updated Dec. 22, 2019, 9:45 p.m.)
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 8f45c60
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java 9045773
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
Diff: https://reviews.apache.org/r/71915/diff/11/
Changes: https://reviews.apache.org/r/71915/diff/10-11/
Testing
-------
Performance test in
1)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
2)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
(Updated Dec. 22, 2019, 7:33 a.m.)
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 8f45c60
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java 9045773
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
Diff: https://reviews.apache.org/r/71915/diff/10/
Changes: https://reviews.apache.org/r/71915/diff/9-10/
Testing
-------
Performance test in
1)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
2)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
(Updated Dec. 20, 2019, 9:27 p.m.)
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 90fcfc3
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
Diff: https://reviews.apache.org/r/71915/diff/9/
Changes: https://reviews.apache.org/r/71915/diff/8-9/
Testing
-------
Performance test in
1)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
2)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
(Updated Dec. 20, 2019, 8:16 p.m.)
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 90fcfc3
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
Diff: https://reviews.apache.org/r/71915/diff/8/
Changes: https://reviews.apache.org/r/71915/diff/7-8/
Testing
-------
Performance test in
1)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
2)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
(Updated Dec. 20, 2019, 8:15 p.m.)
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 90fcfc3
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
Diff: https://reviews.apache.org/r/71915/diff/7/
Changes: https://reviews.apache.org/r/71915/diff/6-7/
Testing
-------
Performance test in
1)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
2)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
(Updated Dec. 18, 2019, 4:52 p.m.)
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
Diff: https://reviews.apache.org/r/71915/diff/6/
Testing (updated)
-------
Performance test in
1)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
2)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
(Updated Dec. 18, 2019, 4:31 p.m.)
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java f8dc211
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
Diff: https://reviews.apache.org/r/71915/diff/6/
Changes: https://reviews.apache.org/r/71915/diff/5-6/
Testing
-------
Performance test in
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
(Updated Dec. 18, 2019, 5:34 a.m.)
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java 8e09490
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
Diff: https://reviews.apache.org/r/71915/diff/5/
Changes: https://reviews.apache.org/r/71915/diff/4-5/
Testing
-------
Performance test in
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
(Updated Dec. 17, 2019, 10:17 p.m.)
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java 7bc94c9
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java fa28716
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java 4c09e68
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java cb201bb
Diff: https://reviews.apache.org/r/71915/diff/4/
Changes: https://reviews.apache.org/r/71915/diff/3-4/
Testing
-------
Performance test in
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
(Updated Dec. 17, 2019, 1:25 a.m.)
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java 3881692
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc0465a
Diff: https://reviews.apache.org/r/71915/diff/3/
Changes: https://reviews.apache.org/r/71915/diff/2-3/
Testing
-------
Performance test in
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li
Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to
return privilege directly
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
-----------------------------------------------------------
(Updated Dec. 15, 2019, 6:43 a.m.)
Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar.
Bugs: sentry-2359
https://issues.apache.org/jira/browse/sentry-2359
Repository: sentry
Description
-------
Right now, the PolicyEngine Interface only returns the list of privileges in the form of String. As a result, every authorization check has to convert the privilege string to privilege object even though the cached privilege objects are of the correct type already.
We should add a new function that returns privilege object directly to avoid the overhead of conversion.
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 5c7f84f
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java de88705
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java 2940a1e
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java 6a8b871
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java 0a0e2f0
sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java 6782089
sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java 94e9919
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java b6a1faa
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java 5b261e3
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java 504b5ea
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java 6c2737a
sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java a819bb0
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java 4bb6d32
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java ddb4ec5
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java 5de3135
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java f2f735b
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java 891c1d9
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java d50a0bc
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java b244dba
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java 222b77a
sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java ccc505f
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java 277f6b3
Diff: https://reviews.apache.org/r/71915/diff/2/
Changes: https://reviews.apache.org/r/71915/diff/1-2/
Testing
-------
Performance test in
Privilege: db[1000], table[10]
Authorizable: db[12000], table[1]
TreePrivilegeCache - total time on list string: 448 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 365 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 717 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 794 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Privilege: db[1000], table[10]
Authorizable: db[12000], table[10]
TreePrivilegeCache - total time on list string: 1302 ms (TestTreePrivilegeCache.testListPrivilegesPerf)
TreePrivilegeCache - total time on list obj: 604 ms (TestTreePrivilegeCache.testListPrivilegeObjectsPerf)
SimplePrivilegeCache - total time on list string: 4436 ms (TestSimplePrivilegeCache.testListPrivilegesPerf)
SimplePrivilegeCache - total time on list obj: 3732 ms (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf)
Thanks,
Na Li