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