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