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