ranger-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From madhan <>
Subject Re: Review Request 70078: RANGER-2341: Support for Incremental policy updates to improve performance of ranger-admin and plugins by optimal building of policy-engine
Date Fri, 01 Mar 2019 07:09:37 GMT

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




agents-common/src/main/java/org/apache/ranger/plugin/util/ServicePolicies.java
Lines 168 (patched)
<https://reviews.apache.org/r/70078/#comment299272>

    tagPolicies is duplicated in line #172 below.



agents-common/src/main/java/org/apache/ranger/plugin/util/ServicePolicies.java
Lines 351 (patched)
<https://reviews.apache.org/r/70078/#comment299273>

    Wouldn't this remove all entries in 'source.tagPolicies'? Consider adding TagPolicies.copyHeader(),
with the same logic as in ServicePolicy.copyHeader() and call it from here.



agents-common/src/main/java/org/apache/ranger/plugin/util/ServicePolicies.java
Lines 361 (patched)
<https://reviews.apache.org/r/70078/#comment299274>

    The logic is difficult to read:-(. Can you please break this up into if/else blocks? Thanks!



security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java
Line 202 (original), 204 (patched)
<https://reviews.apache.org/r/70078/#comment299275>

    for 'static final' memebers use upper case names, just as in lines above. Same for #205
as well.



security-admin/src/main/java/org/apache/ranger/common/RangerServicePoliciesCache.java
Line 83 (original), 83 (patched)
<https://reviews.apache.org/r/70078/#comment299276>

    Given the method wouldn't log anything unless debug is enabled, perhaps line #83 should
be at the entry of this method?



security-admin/src/main/java/org/apache/ranger/db/XXPolicyChangeLogDao.java
Lines 57 (patched)
<https://reviews.apache.org/r/70078/#comment299263>

    For a given serviceId there could be multiple XXPolicyChangeLog records. Calling getSingleResult()
will result in exception if the query returns multiple records. Please review and update.



security-admin/src/main/java/org/apache/ranger/db/XXPolicyChangeLogDao.java
Lines 137 (patched)
<https://reviews.apache.org/r/70078/#comment299266>

    Instead of reading from RangerPolicyService, consider reading the policy from the latest
policy-cache (maintained in Ranger admin).



security-admin/src/main/java/org/apache/ranger/entity/XXSecurityZoneRefGroup.java
Line 212 (original), 212 (patched)
<https://reviews.apache.org/r/70078/#comment299267>

    This file has white-space changes only. Please consider revertig this file.



security-admin/src/main/java/org/apache/ranger/entity/XXSecurityZoneRefResource.java
Line 183 (original), 183 (patched)
<https://reviews.apache.org/r/70078/#comment299268>

    This file has white-space changes only. Please consider revertig this file.



security-admin/src/main/java/org/apache/ranger/entity/XXSecurityZoneRefService.java
Line 184 (original), 184 (patched)
<https://reviews.apache.org/r/70078/#comment299269>

    This file has white-space changes only. Please consider revertig this file.



security-admin/src/main/java/org/apache/ranger/rest/AssetREST.java
Line 527 (original), 527 (patched)
<https://reviews.apache.org/r/70078/#comment299270>

    For legacy REST API, shouldn't the new parameter, 'supportsPolicyDeltas', be 'false'?
The callers will not be able to handle deltas.



security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java
Lines 3724 (patched)
<https://reviews.apache.org/r/70078/#comment299271>

    Consider calling securityZoneInfo.setName(entry.getKey()) - at line #3724 and #3703.



security-admin/src/main/resources/META-INF/jpa_named_queries.xml
Lines 1391 (patched)
<https://reviews.apache.org/r/70078/#comment299264>

    Given use of ">=", consider replacing "findLaterThan" with "findSinceVersion".



security-admin/src/main/resources/META-INF/jpa_named_queries.xml
Lines 1399 (patched)
<https://reviews.apache.org/r/70078/#comment299265>

    Consider renaming "findMoreThan" -> "findIdGreaterThan"


- madhan


On March 1, 2019, 2:58 a.m., Abhay Kulkarni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70078/
> -----------------------------------------------------------
> 
> (Updated March 1, 2019, 2:58 a.m.)
> 
> 
> Review request for ranger, Madhan Neethiraj, Ramesh Mani, Sailaja Polavarapu, and Velmurugan
Periasamy.
> 
> 
> Bugs: RANGER-2341
>     https://issues.apache.org/jira/browse/RANGER-2341
> 
> 
> Repository: ranger
> 
> 
> Description
> -------
> 
> Optimize policy engine construction by applying only the changes to existing policy-engine
> 
> 
> Diffs
> -----
> 
>   agents-common/src/main/java/org/apache/ranger/admin/client/RangerAdminRESTClient.java
dddfbc7fe 
>   agents-common/src/main/java/org/apache/ranger/plugin/contextenricher/RangerTagEnricher.java
2a0797c92 
>   agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicy.java 3bafd5c0f

>   agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicyDelta.java PRE-CREATION

>   agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerServiceDefHelper.java
6df5d8d00 
>   agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngine.java
63fcbd095 
>   agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineCache.java
8642dbee4 
>   agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java
b29f15289 
>   agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineOptions.java
5498545a5 
>   agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyRepository.java
e5c8d0cc4 
>   agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java
289ec9bf2 
>   agents-common/src/main/java/org/apache/ranger/plugin/policyresourcematcher/RangerDefaultPolicyResourceMatcher.java
8d35319ee 
>   agents-common/src/main/java/org/apache/ranger/plugin/service/RangerAuthContext.java
9ae334898 
>   agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java
1c870f7b9 
>   agents-common/src/main/java/org/apache/ranger/plugin/store/ServiceStore.java 01ce9b2e4

>   agents-common/src/main/java/org/apache/ranger/plugin/util/PolicyRefresher.java b5b4f1636

>   agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java
PRE-CREATION 
>   agents-common/src/main/java/org/apache/ranger/plugin/util/RangerRESTClient.java f592ed4e7

>   agents-common/src/main/java/org/apache/ranger/plugin/util/RangerRESTUtils.java f9ef1d301

>   agents-common/src/main/java/org/apache/ranger/plugin/util/RangerResourceTrie.java a2d52a049

>   agents-common/src/main/java/org/apache/ranger/plugin/util/ServicePolicies.java 664523e55

>   agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java
9d9be6c98 
>   agents-common/src/test/resources/policyengine/test_policyengine_hive_incremental_add.json
PRE-CREATION 
>   agents-common/src/test/resources/policyengine/test_policyengine_hive_incremental_delete.json
PRE-CREATION 
>   agents-common/src/test/resources/policyengine/test_policyengine_hive_incremental_update.json
PRE-CREATION 
>   knox-agent/src/main/java/org/apache/ranger/admin/client/RangerAdminJersey2RESTClient.java
d856f898b 
>   security-admin/db/mysql/optimized/current/ranger_core_db_mysql.sql b46a48155 
>   security-admin/db/mysql/patches/038-add-policy-change-log-table.sql PRE-CREATION 
>   security-admin/db/oracle/optimized/current/ranger_core_db_oracle.sql 35c70c7f4 
>   security-admin/db/oracle/patches/038-add-policy-change-log-table.sql PRE-CREATION 
>   security-admin/db/postgres/optimized/current/ranger_core_db_postgres.sql dfa8c829c

>   security-admin/db/postgres/patches/038-add-policy-change-log-table.sql PRE-CREATION

>   security-admin/db/sqlanywhere/optimized/current/ranger_core_db_sqlanywhere.sql 81c6172a6

>   security-admin/db/sqlanywhere/patches/038-add-policy-change-log-table.sql PRE-CREATION

>   security-admin/db/sqlserver/optimized/current/ranger_core_db_sqlserver.sql 845e0891c

>   security-admin/db/sqlserver/patches/038-add-policy-change-log-table.sql PRE-CREATION

>   security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java e1b244d45 
>   security-admin/src/main/java/org/apache/ranger/common/RangerServicePoliciesCache.java
072495212 
>   security-admin/src/main/java/org/apache/ranger/common/db/JPABeanCallbacks.java 70ad44b48

>   security-admin/src/main/java/org/apache/ranger/db/RangerDaoManagerBase.java be1492282

>   security-admin/src/main/java/org/apache/ranger/db/XXPolicyChangeLogDao.java PRE-CREATION

>   security-admin/src/main/java/org/apache/ranger/db/XXServiceDao.java a79ba7c77 
>   security-admin/src/main/java/org/apache/ranger/entity/XXPolicyChangeLog.java PRE-CREATION

>   security-admin/src/main/java/org/apache/ranger/entity/XXSecurityZoneRefGroup.java 0ae6b2ffc

>   security-admin/src/main/java/org/apache/ranger/entity/XXSecurityZoneRefResource.java
3d7197a16 
>   security-admin/src/main/java/org/apache/ranger/entity/XXSecurityZoneRefService.java
a2cacc674 
>   security-admin/src/main/java/org/apache/ranger/rest/AssetREST.java d7089275b 
>   security-admin/src/main/java/org/apache/ranger/rest/PublicAPIsv2.java 0281c94a5 
>   security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java a43d076fd 
>   security-admin/src/main/java/org/apache/ranger/service/RangerSecurityZoneService.java
ab8931910 
>   security-admin/src/main/resources/META-INF/jpa_named_queries.xml a6606010c 
>   security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java bf19efd31

>   security-admin/src/test/java/org/apache/ranger/rest/TestAssetREST.java a1b0e45d5 
>   security-admin/src/test/java/org/apache/ranger/rest/TestServiceREST.java ef4865afd

> 
> 
> Diff: https://reviews.apache.org/r/70078/diff/1/
> 
> 
> Testing
> -------
> 
> - Successfully executed all tests. 
> - Ensured that ranger admin and plugin functionality is indentical with or without policy
deltas enabled.
> - Ran performance test to ensure that JVM Garbage collection behavior is positively impacted
(No Full GC on ranger-admin). 
> - Noticed the policy-engine building time reduced by 80% with ~1800 large policies, with
policy-engine rebuilding within a second in the - plugin when a single policy was updated.

> - With ~3000 large policies, the policy-engine was built in about 2 seconds as compared
to 10 seconds without incremental policy updates.
> 
> 
> Thanks,
> 
> Abhay Kulkarni
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message