----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47872/#review134918 -----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java (line 217) <https://reviews.apache.org/r/47872/#comment199882> Add comment on why you return null here. sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java (line 121) <https://reviews.apache.org/r/47872/#comment199890> add brief comment on what this logic is doing. It's not immediately obvious sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java (line 122) <https://reviews.apache.org/r/47872/#comment199886> Do we need to do a copy here of the KeyValue object or can we just use "part"? KeyValue ctor makes a bunch of copies, should fix that as a follow on item as well. sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java (line 27) <https://reviews.apache.org/r/47872/#comment199887> Can you add comments on the interface, both for the new method added as well as a brief interface comment and comment on implies(). sentry-policy/sentry-policy-indexer/src/main/java/org/apache/sentry/policy/indexer/IndexerWildcardPrivilege.java (line 110) <https://reviews.apache.org/r/47872/#comment199904> Why LinkedList vs ArrayList? sentry-policy/sentry-policy-indexer/src/test/java/org/apache/sentry/policy/indexer/TestCommonPrivilegeForIndexer.java (line 170) <https://reviews.apache.org/r/47872/#comment199891> This "mock" privilege seems to be duplicated a bunch of places. Maybe have a TODO or JIRA to clean this up as a follow on? sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java (line 37) <https://reviews.apache.org/r/47872/#comment199902> Would we ever want someone to use the old listPrivileges API? Shoudl we make it as deprecated or add a comment telling folks not to use it? sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java (line 31) <https://reviews.apache.org/r/47872/#comment199897> Can you update comment to clarify whether this is thread safe or not (it appears it is not, which I think is fine for now). sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java (line 37) <https://reviews.apache.org/r/47872/#comment199899> Expand comments a bit more to explain the purpose of these data structures. sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java (line 38) <https://reviews.apache.org/r/47872/#comment199898> can these be "final" sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java (line 40) <https://reviews.apache.org/r/47872/#comment199906> Do we handle case sensitivity proprly for the keys? Would be good to add some tests around that. sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java (line 57) <https://reviews.apache.org/r/47872/#comment199896> you can simplify the code a bit and remove an extra call to "get()" Set<String> authzPrivileges = cachedAuthzPrivs.get(authzString); if (authzPrivileges == null) { authzPrivileges = new HashSet(); cachedAuthzPrivs.add(authzPrivileges); } authzPrivileges.add(permission); sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java (line 67) <https://reviews.apache.org/r/47872/#comment199900> typo - authoriable -> authorizeable sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java (line 138) <https://reviews.apache.org/r/47872/#comment199901> authorizationHierarchy This is great - should give a nice perf improvement - Lenni Kuff On May 26, 2016, 5:23 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47872/ > ----------------------------------------------------------- > > (Updated May 26, 2016, 5:23 a.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > SENTRY-1291: SimpleCacheProviderBackend.getPrivileges should return the > permission based on authorizationhierarchy > > Change-Id: Ifa38aee17c232c50cd6b992126837a756f3aaa7f > > Changed SimpleCacheProviderBackend.getPrivileges to return the permissions > based on authorizationhierarchy. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHookBase.java > dd16960cb631c8e20e2f23c9ab9d95208d40f595 > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java > c719802150a6dc5e4331af1cdcddabe0329554e9 > > sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java > a616f67b7eeb3b106944137745bb215c04622a93 > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSearch.java > 294091cba0a8989d327de24d50aa81dc3ce238f9 > > sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java > 92e929080fe6fec908141f98d546ef481946a88b > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java > dedd90886e9450054b2cc0a796677318add9b3c0 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java > e9f46094cf79a3f91c953c9b3bbecd3aecc2ebad > > sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java > ee25427bd47834fb94915f7df5d5a0dca850c9bb > > sentry-policy/sentry-policy-indexer/src/main/java/org/apache/sentry/policy/indexer/IndexerWildcardPrivilege.java > 71d2a665dc36cc853cd0a6efef0b654ac044e28f > > sentry-policy/sentry-policy-indexer/src/test/java/org/apache/sentry/policy/indexer/TestCommonPrivilegeForIndexer.java > 2a3bde72aee7eb53150c5524313da18ff201dd7e > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java > 28c5b769d4d77c0f270833d5856f7ee582a950db > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java > b3db752da85a71d8c94f6def7ad0aac600545bd8 > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java > dd0c39ab79470e2477716c5ec4e5aed1d4ec0f55 > > sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java > 79e047ee8037c9463fd812bd57dbad045952a36b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java > 1d1da5e41afb8b32e9aded4b9f06bd285f221a61 > > Diff: https://reviews.apache.org/r/47872/diff/ > > > Testing > ------- > > All existing hive e2e tests. > > > Thanks, > > Hao Hao > >
