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