> 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 > >
