----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74112/#review225210 -----------------------------------------------------------
agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Line 73 (original), 65 (patched) <https://reviews.apache.org/r/74112/#comment313955> Consider renaming policiesIdMap => retMap agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Line 87 (original), 82 (patched) <https://reviews.apache.org/r/74112/#comment313954> Consider replacing #82 - #86 with: RangerPolicy deletedPolicy = policiesMap.remove(policyId); agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Line 123 (original), 112 (patched) <https://reviews.apache.org/r/74112/#comment313956> Shouldn't delta policy be added to policyIdMap (instead of ret), so that it will be considered in #83 above? agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java Lines 121 (patched) <https://reviews.apache.org/r/74112/#comment313957> ret should be empty here, after the change suggested in #112 above. Consider replacing #121 with: ret = new ArrayList<>(policyIdMap.values()); Also, assignment of ret in #63 can be removed. - Madhan Neethiraj On Jan. 29, 2023, 1:01 p.m., Ramachandran Krishnan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74112/ > ----------------------------------------------------------- > > (Updated Jan. 29, 2023, 1:01 p.m.) > > > Review request for ranger, Don Bosco Durai, Kirby Zhou, Abhay Kulkarni, > Madhan Neethiraj, Mehul Parikh, Nikhil P, Pradeep Agrawal, Ramesh Mani, > Selvamohan Neethiraj, Sailaja Polavarapu, Subhrat Chaudhary, and Velmurugan > Periasamy. > > > Bugs: RANGER-3903 > https://issues.apache.org/jira/browse/RANGER-3903 > > > Repository: ranger > > > Description > ------- > > After going through the below code snippets in the master branch > > while (iter.hasNext()) { > RangerPolicy policy = iter.next(); > if (policyId.equals(policy.getId()) && (changeType == > RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE || changeType == > RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE)) { > deletedPolicies.add(policy); > iter.remove(); > } > } > switch (changeType) { > case RangerPolicyDelta.CHANGE_TYPE_POLICY_CREATE: > { > if (CollectionUtils.isNotEmpty(deletedPolicies)) { > LOG.warn("Unexpected: found existing policy for > CHANGE_TYPE_POLICY_CREATE: " + Arrays.toString(deletedPolicies.toArray())); > } > break; > } > case RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE: > { > if (CollectionUtils.isEmpty(deletedPolicies) || > deletedPolicies.size() > 1) { > LOG.warn("Unexpected: found no policy or multiple policies > for CHANGE_TYPE_POLICY_UPDATE: " + > Arrays.toString(deletedPolicies.toArray())); > } > break; > } > case RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE: > { > if (CollectionUtils.isEmpty(deletedPolicies) || > deletedPolicies.size() > 1) { > LOG.warn("Unexpected: found no policy or multiple policies > for CHANGE_TYPE_POLICY_DELETE: " + > Arrays.toString(deletedPolicies.toArray())); > } > break; > } > default: > break; > } > > 1st#improvement: > > From the above code, we iterate delta policies and check if this policy > exists in the existing policy, we add that to deletePolicies list. > > The delta change type condition for created/updated/deleted is added on top > of the if the condition so adding the condition again is not necessary > > 2nd#improvement: > From the above code, we see for each element in the deltas,we iterate > policies and check if this delta policy exists in the existing policy, we add > that to deletePolicies list. > > Solution: > We need to use Map instead of iterating policies for every element of deltas > --> Map<Long,RangerPolicy> policiesIdMap > Building index map key will be policyId and value will be policy associated > with the same policyId > For each policy in the deltas ,we check on the policiesIdMap whether the > same policyId is present or not ?. > if yes, we will add all the associated policy into deletedPolicy variable > and remove the policyId from policiesIdMap > After an end of the iteration,we will iterate the policiesIdMap and get all > the policy associated with policyId and add into result set > > This will give better performance when the policies list is huge. > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java > e9223fe69 > > > Diff: https://reviews.apache.org/r/74112/diff/9/ > > > Testing > ------- > > Tested the below Rest API's to make sure everything works fine > > 1. ServiceREST Rest API :GET /plugins/policies/download/{serviceName} > > 2. ServiceREST Rest API :GET /plugins/secure/policies/download/{serviceName} > > > Thanks, > > Ramachandran Krishnan > >
