----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71542/#review217978 -----------------------------------------------------------
agents-common/src/main/java/org/apache/ranger/plugin/contextenricher/RangerTagEnricher.java Line 253 (original), 256 (patched) <https://reviews.apache.org/r/71542/#comment305399> setServiceTags() method is about 200 lines long:-(. To make it easier to read/review, consider breaking this into multiple methods. agents-common/src/main/java/org/apache/ranger/plugin/contextenricher/RangerTagEnricher.java Lines 502 (patched) <https://reviews.apache.org/r/71542/#comment305400> Can 'enrichedServiceTags' be null? Refer to line #467. security-admin/src/main/java/org/apache/ranger/db/XXPolicyChangeLogDao.java Lines 60 (patched) <https://reviews.apache.org/r/71542/#comment305412> "some record" or "first record"? The while loop below seems to expect the first record to match the given version - line #72. Please review and update. security-admin/src/main/java/org/apache/ranger/db/XXPolicyChangeLogDao.java Lines 67 (patched) <https://reviews.apache.org/r/71542/#comment305411> For better readability, consider replacing "2" with a constant. Please review similar usage in #127 - #130; and other such places. security-admin/src/main/java/org/apache/ranger/db/XXServiceVersionInfoDao.java Line 76 (original), 76 (patched) <https://reviews.apache.org/r/71542/#comment305410> if 'resourceId' and 'tagId' are not expected to be null, consider adding a warn() log to help in troubleshooting. Please review other methods for similr code. security-admin/src/main/java/org/apache/ranger/db/XXServiceVersionInfoDao.java Lines 134 (patched) <https://reviews.apache.org/r/71542/#comment305409> Seems like an incomplete/unnecessary method. Please review and update/remove. security-admin/src/main/java/org/apache/ranger/db/XXServiceVersionInfoDao.java Line 129 (original), 146 (patched) <https://reviews.apache.org/r/71542/#comment305408> lines #146 - #155 can be moved outside the 'for' loop, as value of versionType and tagChangeType remain the same across iterations. Please review and update. security-admin/src/main/java/org/apache/ranger/db/XXTagChangeLogDao.java Lines 55 (patched) <https://reviews.apache.org/r/71542/#comment305407> "some record" or "first record"? The while loop below seems to expect the first record to match the given version - line #67. Please review and update. security-admin/src/main/java/org/apache/ranger/db/XXTagChangeLogDao.java Lines 62 (patched) <https://reviews.apache.org/r/71542/#comment305406> For better readability, consider replacing "2" with a constant. Please review similar usage in #106 - #110; and other such places. security-admin/src/main/java/org/apache/ranger/entity/XXTagChangeLog.java Lines 183 (patched) <https://reviews.apache.org/r/71542/#comment305404> Consider using Objects.equals(), for a simpler comparision code: return Objects.equals(this.id, other.id) && Objects.equals(this.createTime, other.createTime) && Objects.equals(this.serviceId, other.serviceId) && ... security-admin/src/main/java/org/apache/ranger/entity/XXTagChangeLog.java Lines 207 (patched) <https://reviews.apache.org/r/71542/#comment305405> Consider replacing calls to this method with Objects.equals(object1, object2), and remove this method. security-admin/src/main/java/org/apache/ranger/rest/PublicAPIsv2.java Line 529 (original), 529 (patched) <https://reviews.apache.org/r/71542/#comment305403> Similar to deletePolicyDeltas(), a method to deleteTagDeltas() would need to be exposed. Please review and update. security-admin/src/main/java/org/apache/ranger/rest/PublicAPIsv2.java Line 530 (original), 530 (patched) <https://reviews.apache.org/r/71542/#comment305402> reloadServicePoliciesCache - this parameter is no more used. Please review and remove. security-admin/src/main/resources/META-INF/jpa_named_queries.xml Lines 1566 (patched) <https://reviews.apache.org/r/71542/#comment305401> XXTagChangeLog.findByServiceId - this query doesn't seem to be used. Please review and remove if unused. - Madhan Neethiraj On Sept. 25, 2019, 6:26 p.m., Abhay Kulkarni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71542/ > ----------------------------------------------------------- > > (Updated Sept. 25, 2019, 6:26 p.m.) > > > Review request for ranger, Ramesh Mani, Sailaja Polavarapu, and Velmurugan > Periasamy. > > > Bugs: RANGER-2510 > https://issues.apache.org/jira/browse/RANGER-2510 > > > Repository: ranger > > > Description > ------- > > Currently, every change to any tag/service-resource/service-resource->tag > mapping causes complete rebuilding of portions of policy-engine that map > accessed resource to their tags. There are several disadvantages: > 1. Compute time for rebuilding > 2. Large traffic from ranger-admin to each of the plugins > 3. Large load on JVM memory system because of frequent complete rebuilding of > portions of policy-engine. > > It will be more optimal to communicate only the changes to tags and apply > them to existing policy-engine. > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/admin/client/RangerAdminRESTClient.java > 01ee945f3 > > agents-common/src/main/java/org/apache/ranger/plugin/contextenricher/RangerTagEnricher.java > b596992aa > > agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicyDelta.java > 74e7addbe > > agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java > d33f5d36b > > agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyRepository.java > aec325c03 > > agents-common/src/main/java/org/apache/ranger/plugin/policyresourcematcher/RangerDefaultPolicyResourceMatcher.java > 633ec96ac > > agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java > 8de0329f6 > agents-common/src/main/java/org/apache/ranger/plugin/store/TagStore.java > fe4b27817 > > agents-common/src/main/java/org/apache/ranger/plugin/store/TagValidator.java > c334e3963 > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerRESTUtils.java > 310f69dbf > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerResourceTrie.java > 605a74ae4 > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerServiceTagsDeltaUtil.java > PRE-CREATION > agents-common/src/main/java/org/apache/ranger/plugin/util/ServiceTags.java > fed3f1218 > > agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java > 7180675af > > agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngineComparison.java > PRE-CREATION > > agents-common/src/test/resources/policyengine/comparison/fail/myServicePolicies.json > PRE-CREATION > > agents-common/src/test/resources/policyengine/comparison/fail/myServiceTags.json > PRE-CREATION > > agents-common/src/test/resources/policyengine/comparison/fail/otherServicePolicies.json > PRE-CREATION > > agents-common/src/test/resources/policyengine/comparison/fail/otherServiceTags.json > PRE-CREATION > > agents-common/src/test/resources/policyengine/comparison/success/myServicePolicies.json > PRE-CREATION > > agents-common/src/test/resources/policyengine/comparison/success/myServiceTags.json > PRE-CREATION > > agents-common/src/test/resources/policyengine/comparison/success/otherServicePolicies.json > PRE-CREATION > > agents-common/src/test/resources/policyengine/comparison/success/otherServiceTags.json > PRE-CREATION > > agents-common/src/test/resources/policyengine/test_compare_policyengines.json > PRE-CREATION > agents-common/src/test/resources/policyengine/test_policyengine_hive.json > 66b3644b9 > > agents-common/src/test/resources/policyengine/test_policyengine_hive_incremental_add.json > fce157262 > > agents-common/src/test/resources/policyengine/test_policyengine_hive_incremental_delete.json > e60bfe9b7 > > agents-common/src/test/resources/policyengine/test_policyengine_hive_incremental_update.json > dea1c2b74 > > knox-agent/src/main/java/org/apache/ranger/admin/client/RangerAdminJersey2RESTClient.java > db16d73a0 > security-admin/db/mysql/optimized/current/ranger_core_db_mysql.sql > 399fd8aaf > security-admin/db/mysql/patches/043-add-tag-change-log-table.sql > PRE-CREATION > security-admin/db/oracle/optimized/current/ranger_core_db_oracle.sql > 73b91736c > security-admin/db/oracle/patches/043-add-tag-change-log-table.sql > PRE-CREATION > security-admin/db/postgres/optimized/current/ranger_core_db_postgres.sql > 0e8fd9c13 > security-admin/db/postgres/patches/043-add-tag-change-log-table.sql > PRE-CREATION > > security-admin/db/sqlanywhere/optimized/current/ranger_core_db_sqlanywhere.sql > 5d37f08d5 > security-admin/db/sqlanywhere/patches/043-add-tag-change-log-table.sql > PRE-CREATION > security-admin/db/sqlserver/optimized/current/ranger_core_db_sqlserver.sql > 16600e010 > security-admin/db/sqlserver/patches/043-add-tag-change-log-table.sql > PRE-CREATION > > security-admin/src/main/java/org/apache/ranger/biz/RangerTagDBRetriever.java > 394c05ccf > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > e1c45788b > security-admin/src/main/java/org/apache/ranger/biz/TagDBStore.java > 3cc476532 > > security-admin/src/main/java/org/apache/ranger/common/RangerAdminTagEnricher.java > f81184d13 > > security-admin/src/main/java/org/apache/ranger/common/RangerServiceTagsCache.java > 802f42a25 > security-admin/src/main/java/org/apache/ranger/db/RangerDaoManagerBase.java > e4e335fe1 > security-admin/src/main/java/org/apache/ranger/db/XXPolicyChangeLogDao.java > ef436a0d7 > > security-admin/src/main/java/org/apache/ranger/db/XXServiceVersionInfoDao.java > 808170323 > security-admin/src/main/java/org/apache/ranger/db/XXTagChangeLogDao.java > PRE-CREATION > security-admin/src/main/java/org/apache/ranger/entity/XXTagChangeLog.java > PRE-CREATION > security-admin/src/main/java/org/apache/ranger/rest/PublicAPIsv2.java > 2a4c53b48 > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java > 8ee181afb > security-admin/src/main/java/org/apache/ranger/rest/TagREST.java 03d06e248 > > security-admin/src/main/java/org/apache/ranger/service/RangerServiceResourceService.java > f0cb8f4d2 > > security-admin/src/main/java/org/apache/ranger/service/RangerTagDefService.java > c04c3351a > > security-admin/src/main/java/org/apache/ranger/service/RangerTagResourceMapService.java > 89c451ee2 > > security-admin/src/main/java/org/apache/ranger/service/RangerTagService.java > f5060e532 > security-admin/src/main/resources/META-INF/jpa_named_queries.xml 078588a1b > security-admin/src/test/java/org/apache/ranger/rest/TestTagREST.java > 90060b2a6 > > security-admin/src/test/java/org/apache/ranger/service/TestRangerTagDefService.java > 5032c12e5 > > security-admin/src/test/java/org/apache/ranger/service/TestRangerTagResourceMapService.java > 27ec8e140 > > > Diff: https://reviews.apache.org/r/71542/diff/1/ > > > Testing > ------- > > Developed unit tests to verify that incremental policy and tag work as > expected. > > > Thanks, > > Abhay Kulkarni > >
