Sentry-jdk-1.8 - Build # 231 - Failure

[Apache Jenkins Server]

The Apache Jenkins build system has built Sentry-jdk-1.8 (build #231)

Status: Failure

Check console output at https://builds.apache.org/job/Sentry-jdk-1.8/231/ to 
view the results.

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219091
---




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java
Lines 233-236 (patched)


Did you see any issue with out this change?



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
Line 46 (original)


I agree.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
Lines 75-78 (original), 76-84 (patched)


Why should be do this up-casting?

If the below API is not removed from Privilge cache we would need this 
special check.

  Set listPrivileges(Set groups, Set users, 
ActiveRoleSet roleSet,
  Authorizable... authorizationhierarchy);
  
  
  Is there any reason to remove this API. This might even break Impala 
which implements this interface.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 170 (patched)


Second argument here is basically is partIndex. 
You sending a -1 and using it by incrementing by one.
Instead, I think you should just pass 0.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 174 (patched)


First argument here is basically is partIndex. 
You sending a -1 and using it by incrementing by one.
Instead, I think you should just pass 0. 

+1 on vehang's comment.



sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
Lines 217-225 (patched)


Why are you converting the privilge object to string and performening the 
check?

Something like

 private boolean hasOnlyServerPrivilege(Privilege privObj) {
if(privObj.getParts().size() == 1 && 
privObj.getParts().get(0).getKey().equalsIgnoreCase("server")) {
  return privObj.getParts().get(0).getValue().endsWith("+");
}
return false;
  }



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java
Lines 111-151 (patched)


Can you avoid code duplication?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java
Lines 137-150 (patched)


This method is incomplete. If you intent that this API not be used adding a 
comment here may not help.

  @Override
  public ImmutableSet getPrivilegeObjects(Set groups, 
Set users,
ActiveRoleSet roleSet, Authorizable... authorizableHierarchy) {
 throw Exception(); // Appropriate exception
  }
  
  This way if someone calls this method they would know that it is not 
supported.


- kalyan kumar kalvagadda


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

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Vihang Karajgaonkar via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/#review219090
---




sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java
Lines 35 (patched)


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.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
Line 46 (original)


do we need to remove this? This nullifies the objective of creating a 
separate interface FilteredPrivilegeCache



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
Lines 97 (patched)


do we need copyOf?



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
Lines 101 (patched)


instead of returning empty may be throw a UnsupportedOperationException. 
Also you can add a Preconditions.checkState(cacheHandle instanceOf 
FilteredPrivilegeCache) in such a case.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java
Lines 41 (patched)


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.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java
Lines 58 (patched)


can we use privilegeFactory here to create privilege instead of directly 
CommonPrivilege instance?



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 49 (patched)


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.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 57 (patched)


Is the ability to set the privilegeFactory is necessary? Instead can we 
pass the PrivilegeFactory as an argument to the constructor and make it final. 
That would mean that we only need a getPrivilegeFactory method in the in 
FilteredPrivilegeCache interface method instead of setPrivilegeFactory.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 60 (patched)


The reason I suggest not to have a setPrivilegeFactory is because setting 
the privilegeFactory is actually recreating the cache which is an unusual 
pattern to use.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 66-79 (patched)


Why don't we use the input arguments here?



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 67 (patched)


Instead of doing multiple null checks for this variable, may be make sure 
that it is never null initializing this to a HashSet<>() and making it final.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 93 (patched)


why we don't use the groups, users and roleSet arguments. Looks like this 
API is returning all the privileges for all the users for the given 
authorizationheirarchy. Same with the above method.



sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java
Lines 95 (patched)


In the other method calls we are checking if the cachedPrivileges is null 
we return a empty result. May be we should do the same thing to be consistent. 
Since this class is not expected to be thread-safe anyways, why do we need to 
handle this case? I mean, will there be any 

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

---
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 (updated)
-

  
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 
  

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
---

(Updated Dec. 20, 2019, 8:16 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 (updated)
-

  
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 
  

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71915/
---

(Updated Dec. 20, 2019, 8:15 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 (updated)
-

  
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 
  

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
> > Line 852 (original), 852 (patched)
> > 
> >
> > 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.

done


> On Dec. 18, 2019, 10:21 p.m., Vihang Karajgaonkar wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Line 503 (original), 503 (patched)
> > 
> >
> > Make this configurable?

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/PrivilegeCache.java
> > Lines 33 (patched)
> > 
> >
> > 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?

move the new functions to a child interface of PrivilegeCache


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

Re: Review Request 71915: SENTRY-2539: PolicyEngine should be able to return privilege directly

[Na Li via Review Board]

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